You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by ProjectMoon <gi...@git.apache.org> on 2015/10/27 16:31:24 UTC
[GitHub] cloudstack pull request: Make VirtualMachineName into an injectabl...
GitHub user ProjectMoon opened a pull request:
https://github.com/apache/cloudstack/pull/988
Make VirtualMachineName into an injectable service class
This pull request refactors the VirtualMachineName class into an injectable dependency. It is one of the few remaining classes that appears to be spaghetti tangled through many places in the code, and it hasn't been updated in years. These two commits introduce a VirtualMachineNameService which is injected everywhere it's relevant.
The rationale for this is to separate concerns better and remove spaghetti dependencies on static methods. It also opens up the door to easier extensibility in the future.
Some potential issues with this pull request:
* Should the VirtualMachineNameService go into spring-core-managers-context.xml?
* I have added an injection to both VMWare and HypervDirectConnectResource. HyperV already had an inject, but VMWare did not. Both of these are direct agents which means they run in the management server... so injection should work. But I don't know if this violates some guideline or not.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/greenqloud/cloudstack pr-vm-name-destatic
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/988.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #988
----
commit 8095afc07a2657b1363bbeddf80e54931f150f19
Author: jeff <je...@greenqloud.com>
Date: 2015-10-27T11:27:52Z
Make VirtualMachineName into an injectable dependency.
This is one of few remaining classes that is not injectable. In most
places that reference it, it can be injected easily. It should also
be inejctable in the VMWare and HyperV resources which reference it
since they are direct agents.
Rationale: Updating the code to remove static spaghetti dependencies
and paving the way for more internal extensibility.
commit d351ba982a4f4914eb0ad3a77ee67549a328965d
Author: jeff <je...@greenqloud.com>
Date: 2015-10-27T12:04:07Z
Move VirtualMachineName to Service/Impl pattern.
This commit mmakes the VirtualMachineName class into a
VirtualMachineNameService which has an implementation class loaded as
a bean in the core-managers-context. This brings it in line with the
dependency injection patterns used in the CloudStack management
server.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Make VirtualMachineName into an injectabl...
Posted by ProjectMoon <gi...@git.apache.org>.
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/988#issuecomment-151810393
I can create a ticket, yes.
I plan to push more work to this pull request today/tomorrow that moves the VirtualMachineName service and probably the UUID manager into its own module. This would make it very easy for 3rd party developers to override the default VM naming and UUID creation policies in CloudStack by excluding the module and providing their own in its place.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Make VirtualMachineName into an injectabl...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/988#discussion_r43242947
--- Diff: plugins/network-elements/internal-loadbalancer/test/resources/lb_mgr.xml ---
@@ -36,7 +36,7 @@
</list>
</property>
</bean>
-
+
--- End diff --
trailing space
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Make VirtualMachineName into an injectabl...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/988#issuecomment-151658762
not sure what this is @ProjectMoon . I am running a test against your PR overnight and will get back with what I may find, if anything.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Make VirtualMachineName into an injectabl...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/988#issuecomment-151807919
@ProjectMoon impressive work. Can you create a jira ticket and describe it (including functional incentive) and prepend the PR with the ticket id?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9003 Make VirtualMachineName i...
Posted by ProjectMoon <gi...@git.apache.org>.
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/988#issuecomment-152513166
While I haven't pushed new work to this PR yet, I do have a prototype of what I intend to do, and am looking for thoughts on it here.
My plan:
* Move MachineNameService and UUID manager into one interface. Remove the methods from VirtualMachineNameService which are not used. Call this interface ResourceNamingPolicy.
* Create a ResourceNamingPolicy extension registry, and a new module for the default naming policy (which is the UUIDs ACS generates at the moment).
* When using methods from ResourceNamingPolicy, iterate through all registered policies until one returns a name that is not null. This is in line with other things like API authenticators, host planners, etc.
* Expand the use of the ResourceNamingPolicy beyond virtual machines and volumes (which is what VirtualMachineName/UUIDManager touch currently). The plan is to make use of this for these entities to begin with: VMs, volumes, templates, security groups.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9003 Make VirtualMachineName i...
Posted by ProjectMoon <gi...@git.apache.org>.
Github user ProjectMoon closed the pull request at:
https://github.com/apache/cloudstack/pull/988
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Make VirtualMachineName into an injectabl...
Posted by ProjectMoon <gi...@git.apache.org>.
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/988#issuecomment-151553160
I think I also plan on refactoring this pull request further to make the VirtualMachineNameService extensible similar to how some CloudStack adapters work, so the underlying generation of instance hypervisor guest names can be changed, depending on how someone running ACS wants the instance naming policies to work.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Make VirtualMachineName into an injectabl...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/988#issuecomment-151806516
@ProjectMoon THe stuff built and is running tests. You should be able to force push it and it should build. unsure what we are looking for but probably a problem on the build slave.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Make VirtualMachineName into an injectabl...
Posted by ProjectMoon <gi...@git.apache.org>.
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/988#issuecomment-151557578
Looks like build keeps crashing. Should I maybe wait until later to run it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9003 Make VirtualMachineName i...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/988#issuecomment-152516868
@ProjectMoon : sounds good let me know of any more PoC code or further design.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9003 Make VirtualMachineName i...
Posted by ProjectMoon <gi...@git.apache.org>.
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/988#issuecomment-198274020
Closing this as we have finally finished our "real" implementation of this, though it's far beyond the scope of this PR. At some point in the near future we will submit a PR with a full refactoring of how resources are named.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9003 Make VirtualMachineName i...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/988#issuecomment-152118673
@ProjectMoon Thanks for the PR. I cannot judge the actual change you made. I did run some generic tests and they seem to pass just fine:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a tags=advanced,required_hardware=true \
component/test_vpc_redundant.py \
component/test_routers_iptables_default_policy.py \
component/test_routers_network_ops.py \
component/test_vpc_router_nics.py \
smoke/test_loadbalance.py \
smoke/test_internal_lb.py \
smoke/test_ssvm.py \
smoke/test_network.py
```
Result:
```
Create a redundant VPC with two networks with two VMs in each network ... === TestName: test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | Status : SUCCESS ===
ok
Create a redundant VPC with two networks with two VMs in each network and check default routes ... === TestName: test_02_redundant_VPC_default_routes | Status : SUCCESS ===
ok
Test iptables default INPUT/FORWARD policy on RouterVM ... === TestName: test_02_routervm_iptables_policies | Status : SUCCESS ===
ok
Test iptables default INPUT/FORWARD policies on VPC router ... === TestName: test_01_single_VPC_iptables_policies | Status : SUCCESS ===
ok
Stop existing router, add a PF rule and check we can access the VM ... === TestName: test_isolate_network_FW_PF_default_routes | Status : SUCCESS ===
ok
Test redundant router internals ... === TestName: test_RVR_Network_FW_PF_SSH_default_routes | Status : SUCCESS ===
ok
Create a VPC with two networks with one VM in each network and test nics after destroy ... === TestName: test_01_VPC_nics_after_destroy | Status : SUCCESS ===
ok
Create a VPC with two networks with one VM in each network and test default routes ... === TestName: test_02_VPC_default_routes | Status : SUCCESS ===
ok
Test to create Load balancing rule with source NAT ... === TestName: test_01_create_lb_rule_src_nat | Status : SUCCESS ===
ok
Test to create Load balancing rule with non source NAT ... === TestName: test_02_create_lb_rule_non_nat | Status : SUCCESS ===
ok
Test for assign & removing load balancing rule ... === TestName: test_assign_and_removal_lb | Status : SUCCESS ===
ok
Test to verify access to loadbalancer haproxy admin stats page ... === TestName: test02_internallb_haproxy_stats_on_all_interfaces | Status : SUCCESS ===
ok
Test create, assign, remove of an Internal LB with roundrobin http traffic to 3 vm's ... === TestName: test_01_internallb_roundrobin_1VPC_3VM_HTTP_port80 | Status : SUCCESS ===
ok
Test SSVM Internals ... === TestName: test_03_ssvm_internals | Status : SUCCESS ===
ok
Test CPVM Internals ... === TestName: test_04_cpvm_internals | Status : SUCCESS ===
ok
Test stop SSVM ... === TestName: test_05_stop_ssvm | Status : SUCCESS ===
ok
Test stop CPVM ... === TestName: test_06_stop_cpvm | Status : SUCCESS ===
ok
Test reboot SSVM ... === TestName: test_07_reboot_ssvm | Status : SUCCESS ===
ok
Test reboot CPVM ... === TestName: test_08_reboot_cpvm | Status : SUCCESS ===
ok
Test destroy SSVM ... === TestName: test_09_destroy_ssvm | Status : SUCCESS ===
ok
Test destroy CPVM ... === TestName: test_10_destroy_cpvm | Status : SUCCESS ===
ok
Test for port forwarding on source NAT ... === TestName: test_01_port_fwd_on_src_nat | Status : SUCCESS ===
ok
Test for port forwarding on non source NAT ... === TestName: test_02_port_fwd_on_non_src_nat | Status : SUCCESS ===
ok
Test for reboot router ... === TestName: test_reboot_router | Status : SUCCESS ===
ok
Test for Router rules for network rules on acquired public IP ... === TestName: test_network_rules_acquired_public_ip_1_static_nat_rule | Status : FAILED ===
FAIL
Test for Router rules for network rules on acquired public IP ... === TestName: test_network_rules_acquired_public_ip_2_nat_rule | Status : FAILED ===
FAIL
Test for Router rules for network rules on acquired public IP ... === TestName: test_network_rules_acquired_public_ip_2_nat_rule | Status : FAILED ===
FAIL
Test for Router rules for network rules on acquired public IP ... === TestName: test_network_rules_acquired_public_ip_3_Load_Balancer_Rule | Status : FAILED ===
FAIL
----------------------------------------------------------------------
Ran 27 tests in 11424.207s
FAILED (failures=3)
```
The 3 errors at the bottom are due to CLOUDSTACK-8991 and unrelated to this PR.
And:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a tags=advanced,required_hardware=false \
smoke/test_routers.py \
smoke/test_network_acl.py \
smoke/test_privategw_acl.py \
smoke/test_reset_vm_on_reboot.py \
smoke/test_vm_life_cycle.py \
smoke/test_vpc_vpn.py \
smoke/test_service_offerings.py \
component/test_vpc_offerings.py \
component/test_vpc_routers.py
```
Result:
```
Test router internal advanced zone ... === TestName: test_02_router_internal_adv | Status : SUCCESS ===
ok
Test restart network ... === TestName: test_03_restart_network_cleanup | Status : SUCCESS ===
ok
Test router basic setup ... === TestName: test_05_router_basic | Status : SUCCESS ===
ok
Test router advanced setup ... === TestName: test_06_router_advanced | Status : SUCCESS ===
ok
Test stop router ... === TestName: test_07_stop_router | Status : SUCCESS ===
ok
Test start router ... === TestName: test_08_start_router | Status : SUCCESS ===
ok
Test reboot router ... === TestName: test_09_reboot_router | Status : SUCCESS ===
ok
test_privategw_acl (integration.smoke.test_privategw_acl.TestPrivateGwACL) ... === TestName: test_privategw_acl | Status : SUCCESS ===
ok
Test reset virtual machine on reboot ... === TestName: test_01_reset_vm_on_reboot | Status : SUCCESS ===
ok
Test advanced zone virtual router ... === TestName: test_advZoneVirtualRouter | Status : SUCCESS ===
ok
Test Deploy Virtual Machine ... === TestName: test_deploy_vm | Status : SUCCESS ===
ok
Test Multiple Deploy Virtual Machine ... === TestName: test_deploy_vm_multiple | Status : SUCCESS ===
ok
Test Stop Virtual Machine ... === TestName: test_01_stop_vm | Status : SUCCESS ===
ok
Test Start Virtual Machine ... === TestName: test_02_start_vm | Status : SUCCESS ===
ok
Test Reboot Virtual Machine ... === TestName: test_03_reboot_vm | Status : SUCCESS ===
ok
Test destroy Virtual Machine ... === TestName: test_06_destroy_vm | Status : SUCCESS ===
ok
Test recover Virtual Machine ... === TestName: test_07_restore_vm | Status : SUCCESS ===
ok
Test migrate VM ... === TestName: test_08_migrate_vm | Status : SUCCESS ===
ok
Test destroy(expunge) Virtual Machine ... === TestName: test_09_expunge_vm | Status : SUCCESS ===
ok
Test Remote Access VPN in VPC ... === TestName: test_vpc_remote_access_vpn | Status : SUCCESS ===
ok
Test VPN in VPC ... === TestName: test_vpc_site2site_vpn | Status : SUCCESS ===
ok
Test to create service offering ... === TestName: test_01_create_service_offering | Status : SUCCESS ===
ok
Test to update existing service offering ... === TestName: test_02_edit_service_offering | Status : SUCCESS ===
ok
Test to delete service offering ... === TestName: test_03_delete_service_offering | Status : SUCCESS ===
ok
Test for delete account ... === TestName: test_delete_account | Status : SUCCESS ===
ok
Test for Associate/Disassociate public IP address for admin account ... === TestName: test_public_ip_admin_account | Status : SUCCESS ===
ok
Test for Associate/Disassociate public IP address for user account ... === TestName: test_public_ip_user_account | Status : SUCCESS ===
ok
Test for release public IP address ... === TestName: test_releaseIP | Status : SUCCESS ===
ok
Test create VPC offering ... === TestName: test_01_create_vpc_offering | Status : SUCCESS ===
ok
Test VPC offering without load balancing service ... === TestName: test_03_vpc_off_without_lb | Status : SUCCESS ===
ok
Test VPC offering without static NAT service ... === TestName: test_04_vpc_off_without_static_nat | Status : SUCCESS ===
ok
Test VPC offering without port forwarding service ... === TestName: test_05_vpc_off_without_pf | Status : SUCCESS ===
ok
Test VPC offering with invalid services ... === TestName: test_06_vpc_off_invalid_services | Status : SUCCESS ===
ok
Test update VPC offering ... === TestName: test_07_update_vpc_off | Status : SUCCESS ===
ok
Test list VPC offering ... === TestName: test_08_list_vpc_off | Status : SUCCESS ===
ok
test_09_create_redundant_vpc_offering (integration.component.test_vpc_offerings.TestVPCOffering) ... === TestName: test_09_create_redundant_vpc_offering | Status : SUCCESS ===
ok
Test start/stop of router after addition of one guest network ... === TestName: test_01_start_stop_router_after_addition_of_one_guest_network | Status : SUCCESS ===
ok
Test reboot of router after addition of one guest network ... === TestName: test_02_reboot_router_after_addition_of_one_guest_network | Status : SUCCESS ===
ok
Test to change service offering of router after addition of one guest network ... === TestName: test_04_chg_srv_off_router_after_addition_of_one_guest_network | Status : SUCCESS ===
ok
Test destroy of router after addition of one guest network ... === TestName: test_05_destroy_router_after_addition_of_one_guest_network | Status : SUCCESS ===
ok
Test to stop and start router after creation of VPC ... === TestName: test_01_stop_start_router_after_creating_vpc | Status : SUCCESS ===
ok
Test to reboot the router after creating a VPC ... === TestName: test_02_reboot_router_after_creating_vpc | Status : SUCCESS ===
ok
Tests to change service offering of the Router after ... === TestName: test_04_change_service_offerring_vpc | Status : SUCCESS ===
ok
Test to destroy the router after creating a VPC ... === TestName: test_05_destroy_router_after_creating_vpc | Status : SUCCESS ===
ok
----------------------------------------------------------------------
Ran 44 tests in 8741.492s
OK
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---