cloudstack-pull-rats #766 SUCCESS
This pull request looks good

cloudstack-pull-analysis #718 SUCCESS
This pull request looks good

thanks @nitin-maharana maybe we should have a small performance study to show that it actually speeds things up

LGTM (reviewed the changes, did not test it though)

@nitin-maharana I looked at this, seems good. What is the best way to test this or see the difference?

@remibergsma @Runseb
I have below performance analysis, which I did some time back. I am trying to find out the dbdump to do a real test. But below analysis will help to understand the issue and the solution.

Clicking 'Network' tab invokes 3 api as mentioned below.

http://localhost:8080/client/api?command=listZones&response=json&sessionkey=1ddeSxQjc604bdnHJegYqkM6EgA%3D&networktype=Advanced&_=1436356355027 -- 371ms
this is to enable vpc, vpnCustomerGateway menu option.

http://localhost:8080/client/api?command=listNetworks&response=json&sessionkey=1ddeSxQjc604bdnHJegYqkM6EgA%3D&supportedServices=SecurityGroup&listAll=true&details=min&_=1436356355424 -- 18.97s
this is to enable securityGroups menu option.

http://localhost:8080/client/api?command=listNetworks&response=json&sessionkey=1ddeSxQjc604bdnHJegYqkM6EgA%3D&listAll=true&page=1&pagesize=20&_=1436356374437 -- 1.54s
to list networks

Second API call is taking 90% of the total response time.This api fetches all networks and interates over each network and for each network it makes an db call to fetch the services associated with the network. This is an expensive operation.

Solution:

We can fetch listZones of type basic, if present then we can enable securityGroups menu option. For advanced zone with securitygroup service we can reuse first api call and can check "securitygroupsenabled" attribute in response. If present we will enable securityGroups menu option.

http://localhost:8080/client/api?command=listZones&response=json&sessionkey=JwxpVL74Wk6ZS7MvzSw0u6%2FA9eU%3D&networktype=Advanced&_=1436357831259 --256ms

http://localhost:8080/client/api?command=listZones&response=json&sessionkey=JwxpVL74Wk6ZS7MvzSw0u6%2FA9eU%3D&networktype=Basic&_=1436357831901 --11ms

http://localhost:8080/client/api?command=listNetworks&response=json&sessionkey=JwxpVL74Wk6ZS7MvzSw0u6%2FA9eU%3D&listAll=true&page=1&pagesize=20&_=1436357831968 --868s

total : 1.19s

Thanks @sudhansu7. I had also the similar idea. @remibergsma @Runseb please look at the comments of sudhansu. Thanks.

manually verified the fix.
Before the fix:
on list networks page, I see three api calls

1. list zones
2. list networks with SG groups
3. list networks

After the fix:
I see the below api call

1. list zone basic
2. list zone advanced
3. a single list networks call

though I didnt see any performance improvement in my simple setup with 2 networks, the change is working fine and didnt see any issues because of it.

👍 LGTM

@remibergsma 2 LGTM's so it can be merged at your discretion.

This PR has a conflict now, can someone resolve it?

From https://github.com/apache/cloudstack
 * [new ref]         refs/pull/987/head -> pr/987
Auto-merging server/src/com/cloud/network/router/CommandSetupHelper.java
Auto-merging plugins/hypervisors/kvm/test/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java
Auto-merging engine/schema/src/com/cloud/network/dao/NetworkVO.java
Auto-merging api/src/com/cloud/agent/api/to/LoadBalancerTO.java
CONFLICT (content): Merge conflict in api/src/com/cloud/agent/api/to/LoadBalancerTO.java
Automatic merge failed; fix conflicts and then commit the result.
ERROR: Merge failed, aborting.

Hi @remibergsma, I tried rebasing with current master. There is no conflict as such.

I think you were trying to merge the PR #987, But this PR is #897.
And from the log I can see the conflicts are related to PR #987.

Thanks.

Hi @nitin-maharana hmm all those numbers ;-) sorry dude!