CLOUDSTACK-10047: DVSwitch fixes and improvements by yadvr · Pull Request #2293 · apache/cloudstack

yadvr · 2017-10-11T08:08:32Z

This adds a minor feature to accepts security policies while creating network offering. Changes:

Deployed network will have security policies from the network offering
applied on the port group (in vmware environment)
Global settings as fallback when security policies are not defined for a network
offering
Default promiscuous mode security policy set to REJECT as it's the default
for standard/default vswitch

This also allows admins to define a network with vlan range such as vlan://200-400
and use the range to configure vlan-trunking with the range for a portgroup
in dvswitch.

VLAN overlap checks are performed for:

isolated network against existing shared and isolated networks
dedicated vlan ranges for the physical/public network for the zone
shared network against existing isolated network

Notes:

No vlan-range overlap checks are performed when creating shared networks
Multiple vlan id/ranges should include the vlan:// scheme prefix

yadvr · 2017-10-11T08:19:58Z

Notes:

Portgroup creation and update is triggered primarily by StartCommand for a network (when a VM is started, here VM could be either a guest VM or a VR, therefore VM start and VR start triggered by network deployment or network restart with cleanup=true).
While processing StartCommand, the subsystem reconfigures the nics/portgroups/disks etc which is not possible when simply doing a VM reboot/restart from CloudStack, which is why portgroups changes are not applied/propagated when doing VM reboot via CloudStack which send a Vm reboot to vmware.
The global settings will apply for (cloud.public, cloud.guest and cloud.private) portgroups, while if network offering has the security policies defined the defined policies will be applied to cloud.guest (i.e. the guest/vm network specific portgroup) otherwise that will too use the global settings
The feature is primarily intended for new networks and/or network-offerings, and if old/existing network offerings are used then global settings will be used.
The security policies/behaviour is same as vswitches, i.e. promiscuous mode set to false, and others set to true. By default, dvswitch would previously set promiscuous mode to true which would be both an overhead and security issue.

yadvr · 2017-10-11T11:07:35Z

Pinging for review @PaulAngus @nvazquez @DaanHoogland @borisstoyanov @dagsonstebo @resmo @rafaelweingartner @sureshanaparti and others
@blueorangutan package

blueorangutan · 2017-10-11T11:08:30Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2017-10-11T11:35:06Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1150

resmo

good job. extensive work. code lgtm.

resmo · 2017-10-11T11:29:21Z

ui/l10n/en.js

 "label.firewall":"Firewall",
 "label.first.name":"First Name",
 "label.firstname.lower":"firstname",
+"label.forged.trasmits":"Forged Transmits",


typo s/trasmits/transmits

resmo · 2017-10-11T11:29:59Z

ui/scripts/configuration.js

+                                    },
+
+                                    forgedTransmits: {
+                                        label: 'label.forged.trasmits',


yadvr · 2017-10-11T11:51:33Z

Thanks @resmo fixed :)

yadvr · 2017-10-11T11:53:07Z

@blueorangutan test centos7 vmware-55u3

blueorangutan · 2017-10-11T11:53:30Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + vmware-55u3) has been kicked to run smoke tests

rafaelweingartner

Hi @rhtyd I have seen some points that might benefit from improvements.

rafaelweingartner · 2017-10-11T14:39:23Z

server/src/com/cloud/hypervisor/HypervisorGuruBase.java

+            final NetworkVO network = _networkDao.findByUuid(nicTo.getNetworkUuid());
+            if (network != null) {
+                final Map<NetworkOffering.Detail, String> details = networkOfferingDetailsDao.getNtwkOffDetails(network.getNetworkOfferingId());
+                if (details != null) {


What about reducing the cyclomatic complexicity here?
it is a matter of inverting the conditional. Instead of if (true){doSomething}, we can do if(!false){continue} doSomething
This would enable to remove one if inception

We're protecting against NPEs and setting details to a NicTO object when those details are available. We cannot simply continue as the value is being set at lines 161, 163.

You are right about the details, but I think I clicked on the wrong line (sorry for the mistake). I was talking about the if (network != null) check. if if (network == null) you do not do anything, then it is the same as using a continue. I mean, you do a nics[i++] = nicTo;, but that can go inside the condition as well. Duplicated lines are not that great, so let's see what else could be done...

We can do something else, we could extract lines 149-160 to a method; this would improve the readability of the code and enable unit tests and Java docs.

The block converts NicProfile to NicTO, the idea of the changes is to override certain security settings on dvswitch's portgroups if they already don't have the settings, using global settings. I'll explore further refactorings if I get time.

Thanks for both the explanations and for the effort!

BTW: your explanation is great to enrich our code base. This explanation would be awesome in a Javadoc, so people in the future can understand why we are doing this without needing to deeply inspect the code.

rafaelweingartner · 2017-10-11T14:40:44Z

utils/src/main/java/com/cloud/utils/UriUtils.java

+     */
+    public static List<Integer> expandVlanUri(final String vlanAuthority) {
+        final List<Integer> expandedVlans = new ArrayList<>();
+        if (vlanAuthority == null || vlanAuthority.isEmpty()) {


what about StringUtils.isBlank here?

Fixed with Strings.isNullOrEmpty, thanks

rafaelweingartner · 2017-10-11T14:41:13Z

utils/src/main/java/com/cloud/utils/UriUtils.java

+            return expandedVlans;
+        }
+        for (final String vlanPart: vlanAuthority.split(",")) {
+            if (vlanPart == null || vlanPart.isEmpty()) {


what about StringUtils.isBlank here?

Fixed with Strings.isNullOrEmpty, thanks

rafaelweingartner · 2017-10-11T14:43:28Z

utils/src/main/java/com/cloud/utils/UriUtils.java

+                }
+            } else {
+                final Integer value = NumbersUtil.parseInt(range[0], -1);
+                if (value > -1) {


what about a debug message here to say why we are rejecting this value, and displaying the value of range[0], in case range[0] is not a number and the method returns -1

This is used for vlan range checks etc, vlans are always > -1.

rafaelweingartner · 2017-10-11T14:44:47Z

utils/src/main/java/com/cloud/utils/UriUtils.java

+    public static boolean checkVlanUriOverlap(final String vlanRange1, final String vlanRange2) {
+        final List<Integer> vlans1 = expandVlanUri(vlanRange1);
+        final List<Integer> vlans2 = expandVlanUri(vlanRange2);
+        if (vlans1 == null || vlans2 == null) {


if one of them is null, the result is true? This means that they overlap?

We're checking if two vlan ranges (comma separated ranges or values, such as 100-200,300 or 20-30 etc) overlap, if any of them when expanded (i.e. 1-3 expands to 1,2,3) is null, i.e. there is no overlap.

I did not understand the explanations :(

Anyways, looking at the code of expandVlanUri, I did not see a way for it to return null. Worst case scenario it returns an empty list. Do we need this check?

Yes, we need this check.

Do you mind explaining why if the method expandVlanUri returns an empty list in the worst case?

I understand that, but I think we should not over code or engineer. If we worry about the method expandVlanUri returning null, we can do something else to catch this.

What about a test case for expandVlanUri that fails if it returns null? Then, we can remove this check. On thing is to be defensive when a null case can happen, the other is to code expecting someone to make a mistake in the future (for that it is better to write unit test cases).

I think we're over-discussing, not over-engineering :) I'll ping you on respective unit tests.

Ah that as well ;)

I am sorry to bother, but if I do not understand something I keep asking until I can move along.

You could also use !Collections.disjoint(vlans1, vlans2)

@fmaximus fixed.

rafaelweingartner · 2017-10-11T14:47:24Z

engine/schema/src/com/cloud/dc/dao/DataCenterVnetDaoImpl.java


+    private List<DataCenterVnetVO> findOverlappingVnets(final long dcId, final Long physicalNetworkId, final String vnet) {
+        final List<Integer> searchVnets = UriUtils.expandVlanUri(vnet);
+        final List<DataCenterVnetVO> overlappingVnets = new ArrayList<>();


what about inverting this conditional?
if( searchVnets == null || searchVnets.size() == 0){return overlappingVnets;}
This helps to reduce the number of IFs inside of IFs

Fixed, thanks.

blueorangutan · 2017-10-12T01:07:02Z

Trillian test result (tid-1579)
Environment: vmware-55u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 47610 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2293-t1579-vmware-55u3.zip
Intermitten failure detected: /marvin/tests/smoke/test_deploy_vgpu_enabled_vm.py
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermitten failure detected: /marvin/tests/smoke/test_volumes.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Test completed. 58 look OK, 4 have error(s)

Test	Result	Time (s)	Test File
test_01_vpc_remote_access_vpn	`Failure`	151.50	test_vpc_vpn.py
test_01_create_volume	`Failure`	194.87	test_volumes.py
test_02_RVR_Network_FW_PF_SSH_default_routes_egress_false	`Failure`	486.73	test_routers_network_ops.py
test_01_RVR_Network_FW_PF_SSH_default_routes_egress_true	`Failure`	489.86	test_routers_network_ops.py
test_04_rvpc_privategw_static_routes	`Failure`	635.95	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	424.84	test_privategw_acl.py
test_02_vpc_privategw_static_routes	`Failure`	450.06	test_privategw_acl.py
test_01_vpc_privategw_acl	`Failure`	127.16	test_privategw_acl.py
test_08_resize_volume	Skipped	5.12	test_volumes.py
test_07_resize_fail	Skipped	10.26	test_volumes.py
test_09_copy_delete_template	Skipped	0.02	test_templates.py
test_06_copy_template	Skipped	0.00	test_templates.py
test_static_role_account_acls	Skipped	0.02	test_staticroles.py
test_11_ss_nfs_version_on_ssvm	Skipped	0.02	test_ssvm.py
test_01_scale_vm	Skipped	64.44	test_scale_vm.py
test_01_primary_storage_iscsi	Skipped	0.08	test_primary_storage.py
test_vm_nic_adapter_vmxnet3	Skipped	0.00	test_nic_adapter_type.py
test_06_copy_iso	Skipped	0.00	test_iso.py
test_list_ha_for_host_valid	Skipped	0.02	test_hostha_simulator.py
test_list_ha_for_host_invalid	Skipped	0.03	test_hostha_simulator.py
test_list_ha_for_host	Skipped	0.05	test_hostha_simulator.py
test_hostha_enable_feature_without_setting_provider	Skipped	0.03	test_hostha_simulator.py
test_hostha_enable_feature_valid	Skipped	0.03	test_hostha_simulator.py
test_hostha_disable_feature_valid	Skipped	0.02	test_hostha_simulator.py
test_hostha_configure_invalid_provider	Skipped	0.04	test_hostha_simulator.py
test_hostha_configure_default_driver	Skipped	0.05	test_hostha_simulator.py
test_ha_verify_fsm_recovering	Skipped	0.04	test_hostha_simulator.py
test_ha_verify_fsm_fenced	Skipped	0.05	test_hostha_simulator.py
test_ha_verify_fsm_degraded	Skipped	0.05	test_hostha_simulator.py
test_ha_verify_fsm_available	Skipped	0.05	test_hostha_simulator.py
test_ha_multiple_mgmt_server_ownership	Skipped	0.04	test_hostha_simulator.py
test_ha_list_providers	Skipped	0.05	test_hostha_simulator.py
test_ha_enable_feature_invalid	Skipped	0.02	test_hostha_simulator.py
test_ha_disable_feature_invalid	Skipped	0.03	test_hostha_simulator.py
test_ha_configure_enabledisable_across_clusterzones	Skipped	0.03	test_hostha_simulator.py
test_configure_ha_provider_valid	Skipped	0.04	test_hostha_simulator.py
test_configure_ha_provider_invalid	Skipped	0.03	test_hostha_simulator.py
test_remove_ha_provider_not_possible	Skipped	0.05	test_hostha_kvm.py
test_hostha_kvm_host_recovering	Skipped	0.06	test_hostha_kvm.py
test_hostha_kvm_host_fencing	Skipped	0.03	test_hostha_kvm.py
test_hostha_kvm_host_degraded	Skipped	0.04	test_hostha_kvm.py
test_hostha_enable_ha_when_host_in_maintenance	Skipped	0.06	test_hostha_kvm.py
test_hostha_enable_ha_when_host_disconected	Skipped	0.04	test_hostha_kvm.py
test_hostha_enable_ha_when_host_disabled	Skipped	0.04	test_hostha_kvm.py
test_hostha_configure_default_driver	Skipped	0.03	test_hostha_kvm.py
test_disable_oobm_ha_state_ineligible	Skipped	0.04	test_hostha_kvm.py
test_06_verify_guest_lspci_again	Skipped	0.00	test_deploy_virtio_scsi_vm.py
test_05_change_vm_ostype_restart	Skipped	0.00	test_deploy_virtio_scsi_vm.py
test_04_verify_guest_lspci	Skipped	0.00	test_deploy_virtio_scsi_vm.py
test_03_verify_libvirt_attach_disk	Skipped	0.00	test_deploy_virtio_scsi_vm.py
test_02_verify_libvirt_after_restart	Skipped	0.00	test_deploy_virtio_scsi_vm.py
test_01_verify_libvirt	Skipped	0.00	test_deploy_virtio_scsi_vm.py
test_deploy_vgpu_enabled_vm	Skipped	1.17	test_deploy_vgpu_enabled_vm.py

borisstoyanov · 2017-10-12T08:42:45Z

@blueorangutan package

blueorangutan · 2017-10-12T08:43:31Z

@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2017-10-12T09:08:31Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1152

borisstoyanov · 2017-10-12T09:37:18Z

@blueorangutan test

blueorangutan · 2017-10-12T09:37:30Z

@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

yadvr · 2017-10-12T11:25:42Z

utils/src/test/java/com/cloud/utils/UriUtilsTest.java

+        Assert.assertFalse(UriUtils.checkVlanUriOverlap("10-30,45,50,12,31", ""));
+        Assert.assertFalse(UriUtils.checkVlanUriOverlap("10-30,45,50,12,31", "32"));
+        Assert.assertFalse(UriUtils.checkVlanUriOverlap("10,22,111", "12"));
+        Assert.assertFalse(UriUtils.checkVlanUriOverlap("100-200", "30-40,50,201-250"));


@rafaelweingartner unit tests for respective methods in questions are in this file, please see all the above lines.

I had already seen these tests, I did not say anything because in my option they are properly written ;)

So, about that null case, in my option it is already covered, if for some reason someone alters the method to return null, your test cases will catch it (this is great!). That is why I was saying you do not need those null checks.

Sure, sometimes we do want to over-engineer™ :)

Ahaha, sometimes we do, but we should not.

I totally understand when we do, I also exaggerate sometimes, that is why I find it is great a review process to get another set of eyes to look at the problem and the code.

blueorangutan · 2017-10-12T17:01:41Z

Trillian test result (tid-1582)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 26649 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2293-t1582-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Test completed. 60 look OK, 2 have error(s)

Test	Result	Time (s)	Test File
test_01_vpc_remote_access_vpn	`Failure`	60.78	test_vpc_vpn.py
test_04_rvpc_privategw_static_routes	`Failure`	237.96	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	117.19	test_privategw_acl.py
test_02_vpc_privategw_static_routes	`Failure`	182.87	test_privategw_acl.py
test_01_vpc_privategw_acl	`Failure`	56.63	test_privategw_acl.py
test_change_service_offering_for_vm_with_snapshots	Skipped	0.00	test_vm_snapshots.py
test_09_copy_delete_template	Skipped	0.01	test_templates.py
test_06_copy_template	Skipped	0.00	test_templates.py
test_static_role_account_acls	Skipped	0.01	test_staticroles.py
test_11_ss_nfs_version_on_ssvm	Skipped	0.02	test_ssvm.py
test_01_scale_vm	Skipped	0.00	test_scale_vm.py
test_01_primary_storage_iscsi	Skipped	0.06	test_primary_storage.py
test_vm_nic_adapter_vmxnet3	Skipped	0.00	test_nic_adapter_type.py
test_nested_virtualization_vmware	Skipped	0.00	test_nested_virtualization.py
test_06_copy_iso	Skipped	0.00	test_iso.py
test_list_ha_for_host_valid	Skipped	0.02	test_hostha_simulator.py
test_list_ha_for_host_invalid	Skipped	0.02	test_hostha_simulator.py
test_list_ha_for_host	Skipped	0.02	test_hostha_simulator.py
test_hostha_enable_feature_without_setting_provider	Skipped	0.02	test_hostha_simulator.py
test_hostha_enable_feature_valid	Skipped	0.02	test_hostha_simulator.py
test_hostha_disable_feature_valid	Skipped	0.02	test_hostha_simulator.py
test_hostha_configure_invalid_provider	Skipped	0.02	test_hostha_simulator.py
test_hostha_configure_default_driver	Skipped	0.02	test_hostha_simulator.py
test_ha_verify_fsm_recovering	Skipped	0.02	test_hostha_simulator.py
test_ha_verify_fsm_fenced	Skipped	0.02	test_hostha_simulator.py
test_ha_verify_fsm_degraded	Skipped	0.02	test_hostha_simulator.py
test_ha_verify_fsm_available	Skipped	0.02	test_hostha_simulator.py
test_ha_multiple_mgmt_server_ownership	Skipped	0.02	test_hostha_simulator.py
test_ha_list_providers	Skipped	0.02	test_hostha_simulator.py
test_ha_enable_feature_invalid	Skipped	0.02	test_hostha_simulator.py
test_ha_disable_feature_invalid	Skipped	0.02	test_hostha_simulator.py
test_ha_configure_enabledisable_across_clusterzones	Skipped	0.02	test_hostha_simulator.py
test_configure_ha_provider_valid	Skipped	0.02	test_hostha_simulator.py
test_configure_ha_provider_invalid	Skipped	0.02	test_hostha_simulator.py
test_deploy_vgpu_enabled_vm	Skipped	0.02	test_deploy_vgpu_enabled_vm.py
test_3d_gpu_support	Skipped	0.03	test_deploy_vgpu_enabled_vm.py

fmaximus · 2017-10-12T18:02:03Z

server/src/com/cloud/hypervisor/HypervisorGuruBase.java

+            if (network != null) {
+                final Map<NetworkOffering.Detail, String> details = networkOfferingDetailsDao.getNtwkOffDetails(network.getNetworkOfferingId());
+                if (details != null) {
+                    if (!details.containsKey(NetworkOffering.Detail.PromiscuousMode)) {


You might also use putIfAbsent here instead, which was added in Java 8

Fixed. Thanks, good tip.

fmaximus · 2017-10-12T18:15:41Z

utils/src/main/java/com/cloud/utils/UriUtils.java

+    public static boolean checkVlanUriOverlap(final String vlanRange1, final String vlanRange2) {
+        final List<Integer> vlans1 = expandVlanUri(vlanRange1);
+        final List<Integer> vlans2 = expandVlanUri(vlanRange2);
+        if (vlans1 == null || vlans2 == null) {


You could also use !Collections.disjoint(vlans1, vlans2)

sgoeminn

code LGTM!

sgoeminn · 2017-10-13T14:34:21Z

api/src/org/apache/cloudstack/api/command/admin/network/CreateNetworkCmdByAdmin.java

        return vlan;
    }

+    public Boolean getBypassVlanOverlapCheck() {


Is there a reason why we do return a Boolean instead of boolean? (this function can never return null)

Yes, because cmd classes set API args to fields using reflections etc. It's easier to not use native type boolean. When arg is not sent part of the API request, it would be set to null; setting null to boolean will throw an exception.

sgoeminn · 2017-10-13T14:40:56Z

engine/schema/src/com/cloud/dc/dao/DataCenterVnetDaoImpl.java

-    public List<DataCenterVnetVO> findVnet(long dcId, long physicalNetworkId, String vnet) {
+    private List<DataCenterVnetVO> findOverlappingVnets(final long dcId, final Long physicalNetworkId, final String vnet) {
+        final List<Integer> searchVnets = UriUtils.expandVlanUri(vnet);
+        final List<DataCenterVnetVO> overlappingVnets = new ArrayList<>();


You could use searchVnets.isEmpty() instead of size()==0?

Thanks, fixed.

sgoeminn · 2017-10-13T14:52:19Z

vmware-base/src/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java

            networkName = composeCloudNetworkName(namePrefix, vlanId, secondaryvlanId, networkRateMbps, physicalNetwork);

-            if (vlanId != null && !UNTAGGED_VLAN_NAME.equalsIgnoreCase(vlanId)) {
+            if (vlanId != null && !UNTAGGED_VLAN_NAME.equalsIgnoreCase(vlanId) && !vlanId.contains(",") && !vlanId.contains("-")) {


&& !vlanId.contains(",") && !vlanId.contains("-") could be replaced by !StringUtils.containsAny(vlanId, ",-")?

Fixed, thanks.

sgoeminn · 2017-10-13T14:55:00Z

vmware-base/src/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java

                createGCTag = true;
                vid = Integer.parseInt(vlanId);
            }
+            if (vlanId != null && (vlanId.contains(",") || vlanId.contains("-"))) {


could be replaced by StringUtils.containsAny(vlanId, ",-")? Maybe also extract it to a separate boolean because it's checked multiple times?

Fixed, thanks.

borisstoyanov

I've tested this on both vSwitch and dvSwitch vmware environments and it works as expected, marvin smoketests does not show any explicit new failures. LGTM

- Accepts security policies while creating network offering - Deployed network will have security policies from the network offering applied on the port group (in vmware environment) - Global settings as fallback when security policies are not defined for a network offering - Default promiscuous mode security policy set to REJECT as it's the default for standard/default vswitch Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

This allows admins to define a network with comma separated vlan id and vlan range such as vlan://200-400,21,30-50 and use the provided vlan range to configure vlan-trunking for a portgroup in dvswitch based environment. VLAN overlap checks are performed for: - isolated network against existing shared and isolated networks - dedicated vlan ranges for the physical/public network for the zone - shared network against existing isolated network Allow shared networks to bypass vlan overlap checks: This allows admins to create shared networks with a `bypassvlanoverlapcheck` API flag which when set to 'true' will create a shared network without performing vlan overlap checks against isolated network and against the vlans allocated to the datacenter's physical network (vlan ranges). Notes: - No vlan-range overlap checks are performed when creating shared networks - Multiple vlan id/ranges should include the vlan:// scheme prefix Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

yadvr · 2017-10-25T05:48:25Z

I've incorporated feedback from code review, given this has enough LGTMs and test results, I'll merge this as soon as Travis goes green. Thanks everyone for your feedback, review and testings.

yadvr force-pushed the CLOUDSTACK-10047 branch from e9826a6 to 825b817 Compare October 11, 2017 08:16

yadvr force-pushed the CLOUDSTACK-10047 branch from d1491b1 to 84ef76d Compare October 11, 2017 10:56

resmo reviewed Oct 11, 2017

View reviewed changes

yadvr force-pushed the CLOUDSTACK-10047 branch from 84ef76d to a490249 Compare October 11, 2017 11:51

rafaelweingartner reviewed Oct 11, 2017

View reviewed changes

yadvr force-pushed the CLOUDSTACK-10047 branch from a490249 to cb09283 Compare October 12, 2017 05:45

yadvr commented Oct 12, 2017

View reviewed changes

fmaximus reviewed Oct 12, 2017

View reviewed changes

sgoeminn reviewed Oct 13, 2017

View reviewed changes

borisstoyanov approved these changes Oct 17, 2017

View reviewed changes

yadvr added 3 commits October 25, 2017 10:52

Add fixes based on code review

f24ced9

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

yadvr force-pushed the CLOUDSTACK-10047 branch from cb09283 to f24ced9 Compare October 25, 2017 05:47

yadvr merged commit 41fdb88 into apache:master Oct 25, 2017

Conversation

yadvr commented Oct 11, 2017

Uh oh!

yadvr commented Oct 11, 2017

Uh oh!

yadvr commented Oct 11, 2017

Uh oh!

blueorangutan commented Oct 11, 2017

Uh oh!

blueorangutan commented Oct 11, 2017

Uh oh!

resmo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadvr commented Oct 11, 2017

Uh oh!

yadvr commented Oct 11, 2017

Uh oh!

blueorangutan commented Oct 11, 2017

Uh oh!

rafaelweingartner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadvr Oct 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Oct 12, 2017

Uh oh!

borisstoyanov commented Oct 12, 2017

Uh oh!

blueorangutan commented Oct 12, 2017

yadvr Oct 12, 2017 •

edited

Loading