Hi Marcus, Your review is awesomely welcome, Marcus - if such word exist. :)
Since this whole works was carried out along a few months, it was difficult to split formatting, refactoring and functionality. But as you have noticed, besides the Façade/Flyweight pattern applied to the ways command are sent from the VirtualRoutingResource, all the other stuff is really hypervisor/storage independent. Tomorrow I will try to get my KVM environment done so I can also run the same tests against it. Thanks again for the review. Cheers, Wilder On 17 Feb 2015, at 18:01, Marcus <shadow...@gmail.com> wrote: > I've browsed the code, and I don't really see anything > hypervisor-specific for KVM aside from a change to > VirtualRoutingResource to handle the new AbstractConfigItemFacade, > which looks like it's used to pass commands to the virtual router. > Assuming the VR can be configured, there aren't any changes here that > should keep VMs from starting or storage from working as-is. > > As far as code review, this branch contains a lot of noise that makes > it a bit more difficult to review. A large percentage of the diff was > formatting and adding 'final' to variables, etc. Ideally those commits > would be applied individually to master rather than confusing a > functional change with them. > > On Tue, Feb 17, 2015 at 8:07 AM, Marcus <shadow...@gmail.com> wrote: >> Wido, those were my thoughts as well. I haven't looked at the code >> yet, but I'd be surprised if they had to change storage code (or even >> hypervisor-specific code outside of passing along new VR script >> Commands) in any way to accommodate a VR feature, so I hope this is >> just caution. But enough guessing, perhaps I should just go look. >> >> On Tue, Feb 17, 2015 at 6:15 AM, Wido den Hollander <w...@widodh.nl> wrote: >>> -----BEGIN PGP SIGNED MESSAGE----- >>> Hash: SHA1 >>> >>> >>> >>> On 02/17/2015 02:19 PM, Daan Hoogland wrote: >>>> People, >>>> >>>> We are having some internal discussions about this code at >>>> Schuberg Philis. We are (most specifically I am) convinced that we >>>> cannot foresee the ways in which this can break master when merged. >>>> We had only one issue coming back from the community and this was >>>> functional more then an issue. By the number of issues that we find >>>> our selves this can't be right. We had no -1 either so the only >>>> logical conclusion is that no one else is testing this part of the >>>> code. >>>> >>>> We do not test all the code and our quality improvement process is >>>> not up to spec [1] yet. Things are going to stop working in the >>>> master branch when we merge this. I don't like this but I see no >>>> way to prevent this in our present state of affairs. We are adding >>>> as much tests as we can without hurting Schuberg Philis business >>>> and that has a limitation. >>>> >>>> You can read how Wilder's env is set up and Ian's isn't very >>>> different. So let me use a negative formulation this time: If we >>>> merge now the following things will not have been tested: most >>>> hypervisors: kvm vmware hyperv >>> >>> I don't *think* it's a major problem since you changed the VR itself, >>> but the way it is deployed is still the same. In the end it's just a >>> disk for the hypervisor. >>> >>>> storage: all non nfs things >>> >>> I'm not so afraid for this since the storage is handled by the >>> hypervisor and the VR doesn't have any knowledge there. >>> >>>> ipv6 functionality This is not a complete list, I suspect. >>>> >>>> Can I get a lot of +1 in spite of these uncertainties? Can you all >>>> please test as much as possible? >>>> >>> >>> As we discussed on the phone, my test cluster is broken at the moment >>> and it will take me 2~3 weeks to get it up and running again. >>> >>> Wido >>> >>>> I called a merge for this morning but will give it a few more >>>> rebase efforts because of those considerations. >>>> >>>> [1] >>>> https://cwiki.apache.org/confluence/display/CLOUDSTACK/Quality+and+Process+Improvement+Initiative >>>> >>>> On Tue, Feb 17, 2015 at 12:46 AM, Wilder Rodrigues >>>> <wrodrig...@schubergphilis.com> wrote: >>>>> Hi all, >>>>> >>>>> I have been some tests on the branch in order to give you all >>>>> some confidence. >>>>> >>>>> During the tests I found 1 bug related to communication from VM A >>>>> on Tier 1 to VM B on Tier 2 in a Single VPC. I can reproduce the >>>>> bug and it disappears when I convert the Single VPC to a >>>>> redundant one. I already talked to Ian and he is on it. >>>>> >>>>> Results follow below. >>>>> >>>>> Cheers, Wilder >>>>> >>>>> Environment: >>>>> >>>>> Xen 6.2 running on VMware zone within our Betacloud (ACS 4.4.2) >>>>> MySQL running on MacBook Pro Management Server on MacBook Pro >>>>> >>>>> ::: Manual Tests::: >>>>> >>>>> Isolated Networks >>>>> >>>>> * Create Network * Create 2 VMs using new Network * Create FW >>>>> rules * Create PF rules * SSH to the VMs * SSH from one VM onto >>>>> the other in the same isolated network * Destroy Master router * >>>>> Restart the Network * Restart the Network with Clean-up option * >>>>> Repeat steps above >>>>> >>>>> Redundant Isolated Networks >>>>> >>>>> * Create Redundant Network Offering * Create 2 VMs using new >>>>> offering * Create FW rules * Create PF rules * SSH to the VMs * >>>>> SSH from one VM onto the other in the same redundant isolated >>>>> network * Destroy Master router * Restart the Network * Stop the >>>>> Master Router >>>>> >>>>> Single VPC >>>>> >>>>> * Create VPC * Create 2 Tiers * Create ACLS * Create 1 Vm for >>>>> each Tier * Associate 2 IP address * Add PF rules * SSH onto VMs >>>>> * SSH from 1 VM onto another * Restart VPC - Make it redundant * >>>>> Repeat steps above >>>>> >>>>> Redundant VPC >>>>> >>>>> * Create VPC * Create 2 Tiers * Create ACLS * Create 1 Vm for >>>>> each Tier * Associate 2 IP address * Add PF rules * SSH onto VMs >>>>> * SSH from 1 VM onto another * Stop/Destroy the Master Router * >>>>> Observe the Backup router became Master * SSH again onto the VMs >>>>> * Restart VPC (without clean-up) * Observer only 1 new router is >>>>> created * New router is started as Backup * SSH onto VMs * >>>>> Restart VPC (with clean-up) * Observer only 2 new routers are >>>>> created * SSH onto VMs >>>>> >>>>> ::: Automated Tests ::: >>>>> >>>>> Test Create Account and user for that account ... === TestName: >>>>> test_01_create_account | Status : SUCCESS === ok Test Sub domain >>>>> allowed to launch VM when a Domain level zone is created ... === >>>>> TestName: test_01_add_vm_to_subdomain | Status : SUCCESS === ok >>>>> Test delete domain without force option ... === TestName: >>>>> test_DeleteDomain | Status : SUCCESS === ok Test delete domain >>>>> with force option ... === TestName: test_forceDeleteDomain | >>>>> Status : SUCCESS === ok Test update admin details ... === >>>>> TestName: test_updateAdminDetails | Status : SUCCESS === ok Test >>>>> update domain admin details ... === TestName: >>>>> test_updateDomainAdminDetails | Status : SUCCESS === ok Test user >>>>> update API ... === TestName: test_updateUserDetails | Status : >>>>> SUCCESS === ok Test login API with domain ... === TestName: >>>>> test_LoginApiDomain | Status : SUCCESS === ok Test if Login API >>>>> does not return UUID's ... === TestName: >>>>> test_LoginApiUuidResponse | Status : SUCCESS === ok >>>>> >>>>> ---------------------------------------------------------------------- >>>>> >>>>> >>> Ran 9 tests in 1140.977s >>>>> >>>>> OK >>>>> >>>>> Test reset virtual machine on reboot ... === TestName: >>>>> test_01_reset_vm_on_reboot | Status : SUCCESS === ok >>>>> >>>>> ---------------------------------------------------------------------- >>>>> >>>>> >>> Ran 1 test in 216.907s >>>>> >>>>> 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 ... SKIP: At least two hosts should be >>>>> present in the zone for migration Test destroy(expunge) Virtual >>>>> Machine ... === TestName: test_09_expunge_vm | Status : SUCCESS >>>>> === ok >>>>> >>>>> ---------------------------------------------------------------------- >>>>> >>>>> >>> Ran 10 tests in 851.022s >>>>> >>>>> OK (SKIP=1) >>>>> >>>>> Test router internal advanced zone ... SKIP: Marvin configuration >>>>> has no host credentials to check router services 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 >>>>> >>>>> ---------------------------------------------------------------------- >>>>> >>>>> >>> Ran 7 tests in 454.519s >>>>> >>>>> OK (SKIP=1) >>>>> >>>>> 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 >>>>> >>>>> ---------------------------------------------------------------------- >>>>> >>>>> >>> Ran 3 tests in 206.916s >>>>> >>>>> OK >>>>> >>>>> Test VPN in VPC ... === TestName: test_vpc_remote_access_vpn | >>>>> Status : SUCCESS === ok Test VPN in VPC ... === TestName: >>>>> test_vpc_site2site_vpn | Status : SUCCESS === ok >>>>> >>>>> ---------------------------------------------------------------------- >>>>> >>>>> >>> Ran 2 tests in 373.908s >>>>> >>>>> 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.acs.tests.test_vpc_offerings.TestVPCOffering) ... >>>>> === TestName: test_09_create_redundant_vpc_offering | Status : >>>>> SUCCESS === ok >>>>> >>>>> ---------------------------------------------------------------------- >>>>> >>>>> >>> Ran 8 tests in 672.382s >>>>> >>>>> OK >>>>> >>>>> test_privategw_acl >>>>> (integration.acs.tests.test_privategw_acl.TestPrivateGwACL) ... >>>>> === TestName: test_privategw_acl | Status : SUCCESS === ok >>>>> >>>>> ---------------------------------------------------------------------- >>>>> >>>>> >>> Ran 1 test in 90.756s >>>>> >>>>> 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 6 tests in 665.965s >>>>> >>>>> OK >>>>> >>>>> On 16 Feb 2015, at 17:53, Daan Hoogland <daan.hoogl...@gmail.com> >>>>> wrote: >>>>> >>>>>> H, >>>>>> >>>>>> I will merge our feature/systemvm-persistent-config into >>>>>> master. If you have objections please let me know before >>>>>> tomorrow. >>>>>> >>>>>> @john: your comment was addressed in the present day version. >>>>>> >>>>>> -- Daan >>>>> >>>> >>>> >>>> >>> -----BEGIN PGP SIGNATURE----- >>> Version: GnuPG v1 >>> >>> iQIcBAEBAgAGBQJU40zmAAoJEAGbWC3bPspCVHgP/ReiqputXvndXpIy1q9dFBqF >>> wXXtFlqLKAwm7XmtmOrqUdOWPB7fCGPo6G6Tka/divg0BHLxU/Mzfx9vQRXXzGwK >>> 4WROFyOTl0pTLbksJhbTR0pzT4FEfK6EkfIz2S+zd1xaFGlc/puWSjGHs45RS5vm >>> SqNM5knZVo+4nTTMepJFQFEBN4GND8wbTuI3EEjcaodQ1uZM6+3COPaNjj74cXtr >>> K+k7MxSJq0tBBKlCO30Ou0oK/pPn4Az9zYkNgfBoyoJfe8W2PCHS0/f3La3O19Cr >>> lIOjvVoA1zNfC+E8WRhhZC/HNOzme0MiAAPHHkPykWL9xVBObSihkhiu9XehYjKK >>> aK614+Hte6bLD+gra9H01QA1z8Y8ylFxFRTXb+e6fcV6mVReICaQ5tkL50fuTuJ2 >>> HRk+1Ybul35/J3ZW3qQ7rikpF4sQ9XTC6MrCpl3Ix0XopYxWtvkLTHVtH5iLWhzy >>> hwiS2kAcc9F8zsLIl01tTbUMxAJUR6cms2AxInPadYUlNu3FmCUwTaFpV8GIr1VK >>> kdcsEyQdXwXTPCcelCraJ214GVpH3IESqtkeqNDB1F7fPNhJQMyQgVT6fzCVXe4G >>> I9Fo82+2X8x7XEwGZL81nt3D0p+ndG0zd+awWlrR4C7rQ3UJl+SSBIkKtqT8G8XS >>> AA3KCgF8a43lyTNCk1x/ >>> =19tY >>> -----END PGP SIGNATURE-----
