You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by wilderrodrigues <gi...@git.apache.org> on 2015/09/08 17:06:53 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-8814 - Refactoring the configu...
GitHub user wilderrodrigues opened a pull request:
https://github.com/apache/cloudstack/pull/788
CLOUDSTACK-8814 - Refactoring the configuration of Routers and VPC routers nics
Hi there,
I refactored the configureDefaultNics() method in order to split the implementations for Routers and VPC Routers.
The following tests were executed:
* test_vm_life_cycle
* test_routers
* test_vpc_router_nics
* test_vpc_routers
* test_vpc_offerings
@remibergsma @bhaisaab @koushik-das @miguelaferreira @DaanHoogland @karuturi , could you please have a look/test this PR?
Thanks in advance.
Cheers,
Wilder
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ekholabs/cloudstack fix/iso_net-CLOUDSTACK-8814
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/788.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 #788
----
commit 5e9e9b84fb1a4ca029c32e5b1c305124bfa4d4af
Author: wilderrodrigues <wr...@schubergphilis.com>
Date: 2015-09-08T12:33:25Z
CLOUDSTACK-8814 - Refactoring the configuration of Routers and VPC routers nics
----
---
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-8814 - Refactoring the configu...
Posted by nlivens <gi...@git.apache.org>.
Github user nlivens commented on the pull request:
https://github.com/apache/cloudstack/pull/788#issuecomment-138851642
I've tested the patch as well since I was hitting the exact same issue. All is working fine, and interfaces are configured correctly. :+1:
---
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.
---
Re: [GitHub] cloudstack pull request: CLOUDSTACK-8814 - Refactoring
the configu...
Posted by Ian Southam <IS...@schubergphilis.com>.
On 09 Sep 2015, at 12:14, koushik-das <gi...@git.apache.org>> wrote:
One more doubt. Why the ordering of interfaces are different in regular VR and VPC VR? Has it changed recently or was like this since the beginning? And will it cause more issues down the line when VR or VPC VR scripts are modified?
It was always like that.
—
Ian
[GitHub] cloudstack pull request: CLOUDSTACK-8814 - Refactoring the configu...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/788#issuecomment-138865017
One more doubt. Why the ordering of interfaces are different in regular VR and VPC VR? Has it changed recently or was like this since the beginning? And will it cause more issues down the line when VR or VPC VR scripts are modified?
---
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-8814 - Refactoring the configu...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/788
---
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-8814 - Refactoring the configu...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/788#discussion_r39025559
--- Diff: server/src/com/cloud/network/router/NetworkHelperImpl.java ---
@@ -610,20 +608,22 @@ protected HypervisorType getClusterToStartDomainRouterForOvm(final long podId) {
throw new CloudRuntimeException(errMsg);
}
- @Override
- public LinkedHashMap<Network, List<? extends NicProfile>> configureDefaultNics(final RouterDeploymentDefinition routerDeploymentDefinition) throws ConcurrentOperationException, InsufficientAddressCapacityException {
-
- final LinkedHashMap<Network, List<? extends NicProfile>> networks = new LinkedHashMap<Network, List<? extends NicProfile>>(3);
+ protected LinkedHashMap<Network, List<? extends NicProfile>> configureControlNic(final RouterDeploymentDefinition routerDeploymentDefinition) {
+ final LinkedHashMap<Network, List<? extends NicProfile>> controlConfig = new LinkedHashMap<Network, List<? extends NicProfile>>(3);
- // 1) Control network
s_logger.debug("Adding nic for Virtual Router in Control network ");
final List<? extends NetworkOffering> offerings = _networkModel.getSystemAccountNetworkOfferings(NetworkOffering.SystemControlNetwork);
final NetworkOffering controlOffering = offerings.get(0);
- final Network controlConfig = _networkMgr.setupNetwork(s_systemAccount, controlOffering, routerDeploymentDefinition.getPlan(), null, null, false).get(0);
+ final Network controlNic = _networkMgr.setupNetwork(s_systemAccount, controlOffering, routerDeploymentDefinition.getPlan(), null, null, false).get(0);
- networks.put(controlConfig, new ArrayList<NicProfile>());
+ controlConfig.put(controlNic, new ArrayList<NicProfile>());
+
+ return controlConfig;
+ }
+
+ protected LinkedHashMap<Network, List<? extends NicProfile>> configurePublicNic(final RouterDeploymentDefinition routerDeploymentDefinition, final boolean hasGuestNic) {
+ final LinkedHashMap<Network, List<? extends NicProfile>> publicConfig = new LinkedHashMap<Network, List<? extends NicProfile>>(3);
--- End diff --
In the individual configure methods, why is the initial size of the hash map 3? :)
---
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-8814 - Refactoring the configu...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/788#issuecomment-138887313
@koushik-das They have been different for a long time. I think when VPC was invented, they reordered the nics so that the tiers came last (because you can have N of them). Recently in a refactor, the order was also changed in VR. This restores it. If we want the same order, we need to look at all the scripts (cloud-early-config and friends) that use hardcoded nic names. I think this was faster to do.
---
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-8814 - Refactoring the configu...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/788#issuecomment-138861606
Other than one minor comment, LGTM
---
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-8814 - Refactoring the configu...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/788#discussion_r39034047
--- Diff: server/src/com/cloud/network/router/NetworkHelperImpl.java ---
@@ -610,20 +608,22 @@ protected HypervisorType getClusterToStartDomainRouterForOvm(final long podId) {
throw new CloudRuntimeException(errMsg);
}
- @Override
- public LinkedHashMap<Network, List<? extends NicProfile>> configureDefaultNics(final RouterDeploymentDefinition routerDeploymentDefinition) throws ConcurrentOperationException, InsufficientAddressCapacityException {
-
- final LinkedHashMap<Network, List<? extends NicProfile>> networks = new LinkedHashMap<Network, List<? extends NicProfile>>(3);
+ protected LinkedHashMap<Network, List<? extends NicProfile>> configureControlNic(final RouterDeploymentDefinition routerDeploymentDefinition) {
+ final LinkedHashMap<Network, List<? extends NicProfile>> controlConfig = new LinkedHashMap<Network, List<? extends NicProfile>>(3);
- // 1) Control network
s_logger.debug("Adding nic for Virtual Router in Control network ");
final List<? extends NetworkOffering> offerings = _networkModel.getSystemAccountNetworkOfferings(NetworkOffering.SystemControlNetwork);
final NetworkOffering controlOffering = offerings.get(0);
- final Network controlConfig = _networkMgr.setupNetwork(s_systemAccount, controlOffering, routerDeploymentDefinition.getPlan(), null, null, false).get(0);
+ final Network controlNic = _networkMgr.setupNetwork(s_systemAccount, controlOffering, routerDeploymentDefinition.getPlan(), null, null, false).get(0);
- networks.put(controlConfig, new ArrayList<NicProfile>());
+ controlConfig.put(controlNic, new ArrayList<NicProfile>());
+
+ return controlConfig;
+ }
+
+ protected LinkedHashMap<Network, List<? extends NicProfile>> configurePublicNic(final RouterDeploymentDefinition routerDeploymentDefinition, final boolean hasGuestNic) {
+ final LinkedHashMap<Network, List<? extends NicProfile>> publicConfig = new LinkedHashMap<Network, List<? extends NicProfile>>(3);
--- End diff --
Hi @koushik-das ,
I just refactored the code and did not bother about the initial size - it was there before. It actually won't change anything in terms of performance.
Cheers,
Wilder
---
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-8814 - Refactoring the configu...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/788#issuecomment-138887396
My issue is also gone, so LGTM from me as well. Thanks for fixing this.
---
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-8814 - Refactoring the configu...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/788#issuecomment-138846797
I tested the patch, now isolated network VR is coming up and the interfaces are configured in correct order.
eth0 - guest
eth1 - link local
eth2 - public
---
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-8814 - Refactoring the configu...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/788#issuecomment-138594852
Tests results:
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 1331.675s
OK (SKIP=1)
/tmp//MarvinLogs/test_vm_life_cycle_63T0C0/results.txt (END)
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
----------------------------------------------------------------------
Ran 7 tests in 640.780s
OK
/tmp//MarvinLogs/test_routers_5RJ3RR/results.txt (END)
Create a vpc with two networks with two vms in each network ... === TestName: test_01_VPC_nics_after_destroy | Status : SUCCESS ===
ok
----------------------------------------------------------------------
Ran 1 test in 1239.170s
OK
/tmp//MarvinLogs/test_vpc_router_nics_XE5A1M/results.txt (END)
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 8 tests in 1841.429s
OK
/tmp//MarvinLogs/test_vpc_routers_GVQ115/results.txt (END)
Test deploy virtual machines in VPC networks ... === TestName: test_02_deploy_vms_in_vpc_nw | Status : SUCCESS ===
ok
----------------------------------------------------------------------
Ran 1 test in 298.197s
OK
/tmp//MarvinLogs/test_vpc_offerings_OCI7B5/results.txt (END)
---
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-8814 - Refactoring the configu...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/788#issuecomment-138889015
Will proceed with the merge.
Thanks, guys!
---
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.
---