Refactor system VM default network creation by ProjectMoon · Pull Request #1360 · apache/cloudstack

ProjectMoon · 2016-01-22T10:30:10Z

Two small commits which moves the retrieval of the default network for the console proxy and the SSVM into a separate protected method. It's a small change that makes the code more readable/maintainable and also makes the class more suitable for overriding should one want to do this. It's forward-ported from our 4.2 branch.

No new tests since this should not change any functionality, and thus should be covered by the existing unit tests.

Now on the master branch (#1359 was on the wrong branch).

pedro-martins · 2016-01-23T18:23:12Z

Could you replace the logic in your 'if' at lines 673 and 523 "(networks == null || networks.size() == 0)"
to CollectionUtils.isEmpty(networks) from org.apache.commons.collections4.CollectionUtils ?
That can make the logic a bit more legible.

GabrielBrascher · 2016-01-23T18:44:23Z

@ProjectMoon Could you please create a documentation and a test case for this method ("com.cloud.consoleproxy.ConsoleProxyManagerImpl.getDefaultNetworkForCreation(DataCenter)")?

GabrielBrascher · 2016-01-23T19:02:58Z

@ProjectMoon Could you please do the following changes (1, 2, 3 and 4)?

1 - create a method for the code between lines 672 and 676;
2 - create a Javadoc for the new method;
3 - add a test case for the new method.
4 - The same (steps 1, 2 and 3) for the code at lines 678-688.

With that, the code would be clean, well documented and with tests specific for each logic flow (with and without advanced network).

Thank you.

ProjectMoon · 2016-01-25T14:02:31Z

I've added the changes requested, minus the tests (still working on those). I am pushing the changes now for review. @pedro-martins The CollectionUtils library that shows up on my classpath is not from org.apache.commons.collections4, but instead org.apache.commons.collections. Is that what you meant or am I missing a compile-time dependency?

GabrielBrascher · 2016-01-25T14:36:27Z

@ProjectMoon Nice, the method is cleaner and well documented. Just need to add the getDefaultNetworkForBasicZone(DataCenter dc) at the "else" execution flow. Thanks.

Yeah kind of forgot to put it in there. It's there now.

pedro-martins · 2016-01-25T15:14:49Z

Yes, your import is correct, I've done a mistake, the collections4 (new collection version) isn't used by ACS, sry for that ^_^'

ProjectMoon · 2016-01-25T16:20:10Z

Also, where is the best place to put tests for the secondary storage manager? Is there already a test for it?

pedro-martins · 2016-01-25T16:34:24Z

There isn't a test for this class if you want a suggestion, create a new folder named test in cloud-controller-secondary-storage project and create a new package in this folder named org.apache.cloudstack.secondarystorage with a class named SecondaryStorageManagerTest.java

I think that is the correct pattern.
I hope this helps.

ProjectMoon · 2016-01-26T14:23:52Z

I have created the unit tests. They are in one commit. The tests test the protected methods delegated to by getDefaultNetworkForCreation. It tests all 4 possible code paths + 2 invalid input types.

ProjectMoon · 2016-01-27T13:45:54Z

Seems the Jenkins build is complaining about license missing. Don't see what file it is, but I assume it's the new unit test file?

GabrielBrascher · 2016-01-27T13:57:39Z

@ProjectMoon Yes, just need to add the license in "SecondaryStorageManagerTest" class.

ProjectMoon · 2016-01-27T16:31:07Z

License has been added.

GabrielBrascher · 2016-01-27T17:15:12Z

@ProjectMoon The code is better now, well documented and with tests.
LGTM.

ProjectMoon · 2016-01-29T09:55:21Z

Should the three commits be squashed? Or left in three? I suggest perhaps splitting the unit test commit into two, and putting each unit test with its corresponding refactor commit, so the end result is two commits, each refactoring one part of system VM network selection and providing an associated unit test.

rodrigo93 · 2016-01-31T00:44:48Z

Hi @ProjectMoon,
What a great work you did here.
Splitting the PR in two seems a good idea, in my point of view, since you create two tests cases for distinct classes and edited each one.
Consider squashing the commits as well after that.
I would suggest you to make a JIRA and add it's tag to the title of this PR. So it would be something like: CLOUDSTACK-(number): Refactor system VM default network creation.
Thanks!

GabrielBrascher · 2016-02-01T12:09:57Z

@rodrigo93 I think that a JIRA ticket wouldn't help much, considering that JIRA is for report issues (not PRs). I think that the point here is to analyze the code and give suggestions that could improve this code; if the work done by @ProjectMoon is ok, then a LGTM would do.

ProjectMoon · 2016-02-01T13:01:28Z

I would prefer just leaving one pull request here, but with two commits. Each commit would have the file changes + unit tests for the SSVM and CPVM, respectively. Not sure a JIRA ticket is really needed for this in my opinion.

rodrigo93 · 2016-02-01T14:54:11Z

@GabrielBrascher @ProjectMoon, You are right. I thought I saw a some section in Jira for minor improvements, but I was wrong. Besides that, the code and work done here LGTM.

ProjectMoon · 2016-02-03T11:42:43Z

I now consider the pull request ready to be merged. The unit test commit has been split out and merged into the two commits for the console proxy and SSVM changes, so each commit refactors the class and provides new unit tests for the refactoring. The PR has also been rebased against latest master.

ProjectMoon · 2016-02-18T10:36:03Z

PR rebased against latest master.

It is now broken into separate methods for more readability and flexibility. Each zone type (basic, advanced) has its own method for getting the default network when creating the VM.

It is now broken into separate methods based on zone. Each zone type (basic, advanced) has its own method to select the default network when starting the VM.

koushik-das · 2016-04-06T08:52:45Z

+ }
+
+ @Test
+ public void getDefaultNetworkForAdvancedSG() {


I don't see any security group related check in getDefaultNetworkForBasicZone(). Why do you need tests for SG/non-SG in case basic zone?

To cover all scenarios. Even if there is no security group-related checks in getDefaultNetworkForBasicZone, client code doesn't necessarily know this and the unit tests should make sure all possible combinations work.

@ProjectMoon The unit tests getDefaultNetworkForBasicNonSG() and getDefaultNetworkForBasicSG() are same since SG has no impact on the method being tested.
The line "when(dc.isSecurityGroupEnabled()).thenReturn(true or false);" doesn't impact test outcome. Thats what I mentioned in my initial comment.

swill · 2016-05-18T04:11:49Z

CI RESULTS

Tests Run: 85
Skipped: 0
Failed: 0
Errors: 1
Duration: 4h 19m 39s

Summary of the problem(s):

ERROR: Test create, assign, remove of an Internal LB with roundrobin http traffic to 3 vm's in a Redundant VPC
----------------------------------------------------------------------
Traceback (most recent call last):
File "/data/git/cs1/cloudstack/test/integration/smoke/test_internal_lb.py", line 617, in test_02_internallb_roundrobin_1RVPC_3VM_HTTP_port80
self.execute_internallb_roundrobin_tests(redundant_vpc_offering)
File "/data/git/cs1/cloudstack/test/integration/smoke/test_internal_lb.py", line 685, in execute_internallb_roundrobin_tests
client_vm, applb.sourceipaddress, max_http_requests)
File "/data/git/cs1/cloudstack/test/integration/smoke/test_internal_lb.py", line 526, in run_ssh_test_accross_hosts
ssh_client = self.get_ssh_client(clienthost, 8)
File "/data/git/cs1/cloudstack/test/integration/smoke/test_internal_lb.py", line 497, in get_ssh_client
self.fail("Unable to create ssh connection: " % e)
TypeError: not all arguments converted during string formatting
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_network_BTA0HA/results.txt

Associated Uploads

/tmp/MarvinLogs/DeployDataCenter__May_18_2016_00_49_48_L29IC8:

/tmp/MarvinLogs/test_network_BTA0HA:

/tmp/MarvinLogs/test_vpc_routers_BLKRI3:

Uploads will be available until 2016-07-18 02:00:00 +0200 CEST

Comment created by upr comment.

swill · 2016-05-18T04:28:26Z

I think this one is ready. The CI is coming back pretty clean and I have the required code review.

swill · 2016-05-20T16:40:13Z

CI RESULTS

Tests Run: 82
Skipped: 0
Failed: 1
Errors: 4
Duration: 10h 22m 36s

Summary of the problem(s):

FAIL: Test destroy(expunge) Virtual Machine
----------------------------------------------------------------------
Traceback (most recent call last):
File "/data/git/cs1/cloudstack/test/integration/smoke/test_vm_life_cycle.py", line 646, in test_09_expunge_vm
self.assertEqual(list_vm_response,None,"Check Expunged virtual machine is in listVirtualMachines response")
AssertionError: Check Expunged virtual machine is in listVirtualMachines response
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_vpc_routers_NMPJUY/results.txt

ERROR: Test to verify access to loadbalancer haproxy admin stats page
----------------------------------------------------------------------
Traceback (most recent call last):
File "/data/git/cs1/cloudstack/test/integration/smoke/test_internal_lb.py", line 854, in tearDown
raise Exception("Cleanup failed with %s" % e)
Exception: Cleanup failed with Job failed: {jobprocstatus : 0, created : u'2016-05-20T12:35:56+0200', jobresult : {errorcode : 530, errortext : u'Failed to delete network'}, cmd : u'org.apache.cloudstack.api.command.user.network.DeleteNetworkCmd', userid : u'af61c686-1e47-11e6-9347-5254001daa61', jobstatus : 2, jobid : u'dd08012d-1a5d-40cd-a569-4fc4b474b235', jobresultcode : 530, jobresulttype : u'object', jobinstancetype : u'Network', accountid : u'af61aacc-1e47-11e6-9347-5254001daa61'}
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_network_0JEOCP/results.txt

ERROR: test suite for <class 'integration.smoke.test_vpc_vpn.TestRVPCSite2SiteVpn'>
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 209, in run
self.setUp()
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 292, in setUp
self.setupContext(ancestor)
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 315, in setupContext
try_run(context, names)
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/util.py", line 471, in try_run
return func()
File "/data/git/cs1/cloudstack/test/integration/smoke/test_vpc_vpn.py", line 835, in setUpClass
cls.template.download(cls.apiclient)
File "/usr/lib/python2.7/site-packages/marvin/lib/base.py", line 1350, in download
elif 'Downloaded' in template.status:
TypeError: argument of type 'NoneType' is not iterable
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_network_0JEOCP/results.txt

ERROR: test suite for <class 'integration.smoke.test_vpc_vpn.TestVpcRemoteAccessVpn'>
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 209, in run
self.setUp()
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 292, in setUp
self.setupContext(ancestor)
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 315, in setupContext
try_run(context, names)
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/util.py", line 471, in try_run
return func()
File "/data/git/cs1/cloudstack/test/integration/smoke/test_vpc_vpn.py", line 293, in setUpClass
cls.template.download(cls.apiclient)
File "/usr/lib/python2.7/site-packages/marvin/lib/base.py", line 1350, in download
elif 'Downloaded' in template.status:
TypeError: argument of type 'NoneType' is not iterable
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_network_0JEOCP/results.txt

ERROR: test suite for <class 'integration.smoke.test_vpc_vpn.TestVpcSite2SiteVpn'>
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 209, in run
self.setUp()
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 292, in setUp
self.setupContext(ancestor)
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 315, in setupContext
try_run(context, names)
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/util.py", line 471, in try_run
return func()
File "/data/git/cs1/cloudstack/test/integration/smoke/test_vpc_vpn.py", line 472, in setUpClass
cls.template.download(cls.apiclient)
File "/usr/lib/python2.7/site-packages/marvin/lib/base.py", line 1350, in download
elif 'Downloaded' in template.status:
TypeError: argument of type 'NoneType' is not iterable
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_network_0JEOCP/results.txt

Associated Uploads

/tmp/MarvinLogs/DeployDataCenter__May_20_2016_07_02_38_R64774:

/tmp/MarvinLogs/test_network_0JEOCP:

/tmp/MarvinLogs/test_vpc_routers_NMPJUY:

Uploads will be available until 2016-07-20 02:00:00 +0200 CEST

Comment created by upr comment.

swill · 2016-05-20T16:41:27Z

Ran again because the error was not something I had seen often. The errors in the new run are not related to this PR.

…k-creation Refactor system VM default network creationTwo small commits which moves the retrieval of the default network for the console proxy and the SSVM into a separate protected method. It's a small change that makes the code more readable/maintainable and also makes the class more suitable for overriding should one want to do this. It's forward-ported from our 4.2 branch. No new tests since this should not change any functionality, and thus should be covered by the existing unit tests. Now on the master branch (#1359 was on the wrong branch). * pr/1360: Refactor ssvm default network retrieval. Refactor console proxy default network retrieval. Signed-off-by: Will Stevens <williamstevens@gmail.com>

bvbharatk · 2016-05-26T17:35:22Z

ACS CI BVT Run

Sumarry:
Build Number 58
Hypervisor xenserver
NetworkType Advanced
Passed=67
Failed=4
Skipped=3

Link to logs Folder (search by build_no):https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0

Failed tests:

test_vpc_vpn.py
- test_01_vpc_remote_access_vpn Failed
test_service_offerings.py
- ContextSuite context=TestServiceOfferings>:setup Failing since 2 runs
test_deploy_vm_iso.py
- test_deploy_vm_from_iso Failed
test_volumes.py
- test_06_download_detached_volume Failed

Skipped tests:
test_vm_nic_adapter_vmxnet3
test_static_role_account_acls
test_deploy_vgpu_enabled_vm

Passed test suits:
test_deploy_vm_with_userdata.py
test_affinity_groups_projects.py
test_portable_publicip.py
test_over_provisioning.py
test_global_settings.py
test_scale_vm.py
test_routers_iptables_default_policy.py
test_routers.py
test_reset_vm_on_reboot.py
test_snapshots.py
test_deploy_vms_with_varied_deploymentplanners.py
test_list_ids_parameter.py
test_public_ip_range.py
test_multipleips_per_nic.py
test_regions.py
test_affinity_groups.py
test_network_acl.py
test_pvlan.py
test_nic.py
test_deploy_vm_root_resize.py
test_resource_detail.py
test_secondary_storage.py
test_vm_life_cycle.py
test_disk_offerings.py

GabrielBrascher reviewed Jan 23, 2016
View reviewed changes

GabrielBrascher reviewed Jan 25, 2016
View reviewed changes

ProjectMoon mentioned this pull request Jan 29, 2016
Emit event UUIDs on template deletion #1378
Closed

cristofolini mentioned this pull request Feb 7, 2016
CLOUDSTACK-9252: Support configurable NFS version for Secondary Storage mounts #1361
Merged

jeff added 2 commits March 31, 2016 10:41

Refactor console proxy default network retrieval.
3e2c18a
It is now broken into separate methods for more readability and flexibility. Each zone type (basic, advanced) has its own method for getting the default network when creating the VM.

Refactor ssvm default network retrieval.
8971875
It is now broken into separate methods based on zone. Each zone type (basic, advanced) has its own method to select the default network when starting the VM.

koushik-das reviewed Apr 6, 2016
View reviewed changes

anshul1886 mentioned this pull request May 18, 2016
Made the adding new keyboard language support easier #669
Closed

asfgit merged commit 8971875 into apache:masterMay 26, 2016

ProjectMoon deleted the pr-refactor-system-vm-network-creation branch May 26, 2016 11:16

Uh oh!

Conversation

ProjectMoon commented Jan 22, 2016

Uh oh!

pedro-martins commented Jan 23, 2016

Uh oh!

GabrielBrascherJan 23, 2016

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher commented Jan 23, 2016

Uh oh!

ProjectMoon commented Jan 25, 2016

Uh oh!

GabrielBrascherJan 25, 2016

Choose a reason for hiding this comment

Uh oh!

ProjectMoonJan 25, 2016

Choose a reason for hiding this comment

Uh oh!

pedro-martins commented Jan 25, 2016

Uh oh!

ProjectMoon commented Jan 25, 2016

Uh oh!

pedro-martins commented Jan 25, 2016

Uh oh!

ProjectMoon commented Jan 26, 2016

Uh oh!

ProjectMoon commented Jan 27, 2016

Uh oh!

GabrielBrascher commented Jan 27, 2016

Uh oh!

ProjectMoon commented Jan 27, 2016

Uh oh!

GabrielBrascher commented Jan 27, 2016

Uh oh!

ProjectMoon commented Jan 29, 2016

Uh oh!

rodrigo93 commented Jan 31, 2016

Uh oh!

GabrielBrascher commented Feb 1, 2016

Uh oh!

ProjectMoon commented Feb 1, 2016

Uh oh!

rodrigo93 commented Feb 1, 2016

Uh oh!

ProjectMoon commented Feb 3, 2016

Uh oh!

ProjectMoon commented Feb 18, 2016

Uh oh!

koushik-dasApr 6, 2016

Choose a reason for hiding this comment

Uh oh!

ProjectMoonApr 7, 2016

Choose a reason for hiding this comment

Uh oh!

koushik-dasApr 7, 2016

Choose a reason for hiding this comment

Uh oh!

swill commented May 18, 2016

CI RESULTS

Uh oh!

swill commented May 18, 2016

Uh oh!

swill commented May 20, 2016

CI RESULTS

Uh oh!

swill commented May 20, 2016

Uh oh!

bvbharatk commented May 26, 2016

ACS CI BVT Run

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants