Mailing-List: contact dev-help@cloudstack.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cloudstack.apache.org
Content-Type: multipart/alternative;
 boundary="===============4794415270935638188=="
MIME-Version: 1.0
Subject: Re: Review Request 12941: [GSoC] refactor gre controller
From: "Hugo Trippaers" <htrippaers@schubergphilis.com>
To: "Sebastien Goasguen" <runseb@gmail.com>,
 "Hugo Trippaers" <htrippaers@schubergphilis.com>
Cc: "cloudstack" <cloudstack-dev@incubator.apache.org>,
 "tuna" <ng.tuna@gmail.com>
Date: Thu, 25 Jul 2013 09:57:32 -0000
Message-ID: <20130725095732.32328.85299@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Hugo Trippaers" <noreply@reviews.apache.org>
References: <20130725090539.32328.556@reviews.apache.org>
In-Reply-To: <20130725090539.32328.556@reviews.apache.org>
Reply-To: "Hugo Trippaers" <htrippaers@schubergphilis.com>

--===============4794415270935638188==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12941/#review23827
-----------------------------------------------------------


Heya,

There is some feedback from reading the diffs. In general the patch looks good, i'll try and implement a setup based on this so i can test it. I'm assuming that with this patch we can get the L2 part of the GRE based SDN working?

Can you explain what testing you did to prove that this code works? What did your setup look like and what tests did you execute?

Cheers,

Hugo


api/src/com/cloud/network/Network.java
<https://reviews.apache.org/r/12941/#comment47666>

    Please double check the code style. Not tabs, 4 spaces indent for example. The full style guide is available on confluence.


plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java
<https://reviews.apache.org/r/12941/#comment47667>

    Please check the style guide :-) No wild card imports.
    
    http://cloudstack.apache.org/develop/coding-conventions.html


plugins/network-elements/ovs/src/com/cloud/network/element/OvsElement.java
<https://reviews.apache.org/r/12941/#comment47668>

    You need the place holders, but to keep that patch clean it's often better to do that in an other patch.
    
    One patch with the functionality that is ready and can be used. Another one that will enabled place holders for future usage.
    
    Now we can't apply this patch to master because you advertise functionality that is not really implemented.


plugins/network-elements/ovs/src/com/cloud/network/guru/OvsGuestNetworkGuru.java
<https://reviews.apache.org/r/12941/#comment47670>

    This should be gone, determining if the GRE tunnel manager should be used must happen based on the settings of the physical network isolation type "GRE".


setup/db/create-schema.sql
<https://reviews.apache.org/r/12941/#comment47671>

    Please don't make changes to this file.  Changes should be processed by the upgrade scripts.
    
    See http://markmail.org/message/uo7yq2qws2amjkfu


- Hugo Trippaers


On July 25, 2013, 9:05 a.m., tuna wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12941/
> -----------------------------------------------------------
> 
> (Updated July 25, 2013, 9:05 a.m.)
> 
> 
> Review request for cloudstack, Sebastien Goasguen and Hugo Trippaers.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
> I made an update to refactor gre controller:
> 
> + remove ovs_devices table, because we'll have an ODL plugin separately.
> + move command/answer to new package: com.cloud.agent.api
> + add Connectivity service checking
> + add new NetworkProvider: Ovs
> + add L3 services to Ovs Capabilities
> + add L3 services prototype code.
> 
> Next step:
> + L3 services implement with VirtualRouter
> + ODL plugin
> 
> 
> Diffs
> -----
> 
>   api/src/com/cloud/network/Network.java a06208b 
>   client/tomcatconf/applicationContext.xml.in 60f1e30 
>   plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java 30b0521 
>   plugins/network-elements/ovs/src/com/cloud/agent/api/OvsCreateGreTunnelAnswer.java PRE-CREATION 
>   plugins/network-elements/ovs/src/com/cloud/agent/api/OvsCreateGreTunnelCommand.java PRE-CREATION 
>   plugins/network-elements/ovs/src/com/cloud/agent/api/OvsCreateTunnelAnswer.java PRE-CREATION 
>   plugins/network-elements/ovs/src/com/cloud/agent/api/OvsCreateTunnelCommand.java PRE-CREATION 
>   plugins/network-elements/ovs/src/com/cloud/agent/api/OvsDeleteFlowCommand.java PRE-CREATION 
>   plugins/network-elements/ovs/src/com/cloud/agent/api/OvsDestroyBridgeCommand.java PRE-CREATION 
>   plugins/network-elements/ovs/src/com/cloud/agent/api/OvsDestroyTunnelCommand.java PRE-CREATION 
>   plugins/network-elements/ovs/src/com/cloud/agent/api/OvsFetchInterfaceAnswer.java PRE-CREATION 
>   plugins/network-elements/ovs/src/com/cloud/agent/api/OvsFetchInterfaceCommand.java PRE-CREATION 
>   plugins/network-elements/ovs/src/com/cloud/agent/api/OvsSetTagAndFlowAnswer.java PRE-CREATION 
>   plugins/network-elements/ovs/src/com/cloud/agent/api/OvsSetTagAndFlowCommand.java PRE-CREATION 
>   plugins/network-elements/ovs/src/com/cloud/agent/api/OvsSetupBridgeCommand.java PRE-CREATION 
>   plugins/network-elements/ovs/src/com/cloud/agent/api/StartupOvsCommand.java PRE-CREATION 
>   plugins/network-elements/ovs/src/com/cloud/api/response/OvsDeviceResponse.java c0901b2 
>   plugins/network-elements/ovs/src/com/cloud/network/commands/AddOvsDeviceCmd.java 1abc324 
>   plugins/network-elements/ovs/src/com/cloud/network/commands/DeleteOvsDeviceCmd.java 87eedfb 
>   plugins/network-elements/ovs/src/com/cloud/network/commands/ListOvsDevicesCmd.java 2adb33a 
>   plugins/network-elements/ovs/src/com/cloud/network/element/OvsElement.java 0ea6b52 
>   plugins/network-elements/ovs/src/com/cloud/network/element/OvsElementService.java b55fe6b 
>   plugins/network-elements/ovs/src/com/cloud/network/guru/OvsGuestNetworkGuru.java bbdf110 
>   plugins/network-elements/ovs/src/com/cloud/network/ovs/OvsApi.java b533312 
>   plugins/network-elements/ovs/src/com/cloud/network/ovs/OvsApiException.java 20603e0 
>   plugins/network-elements/ovs/src/com/cloud/network/ovs/OvsCreateGreTunnelAnswer.java 5f0f8c1 
>   plugins/network-elements/ovs/src/com/cloud/network/ovs/OvsCreateGreTunnelCommand.java e2cd2d8 
>   plugins/network-elements/ovs/src/com/cloud/network/ovs/OvsCreateTunnelAnswer.java fc2eb8a 
>   plugins/network-elements/ovs/src/com/cloud/network/ovs/OvsCreateTunnelCommand.java 1ececa0 
>   plugins/network-elements/ovs/src/com/cloud/network/ovs/OvsDeleteFlowCommand.java 2a6d5d7 
>   plugins/network-elements/ovs/src/com/cloud/network/ovs/OvsDestroyBridgeCommand.java 8be5586 
>   plugins/network-elements/ovs/src/com/cloud/network/ovs/OvsDestroyTunnelCommand.java 4594d99 
>   plugins/network-elements/ovs/src/com/cloud/network/ovs/OvsFetchInterfaceAnswer.java 1ee6606 
>   plugins/network-elements/ovs/src/com/cloud/network/ovs/OvsFetchInterfaceCommand.java c27daf0 
>   plugins/network-elements/ovs/src/com/cloud/network/ovs/OvsSetTagAndFlowAnswer.java ba16839 
>   plugins/network-elements/ovs/src/com/cloud/network/ovs/OvsSetTagAndFlowCommand.java 17121a0 
>   plugins/network-elements/ovs/src/com/cloud/network/ovs/OvsSetupBridgeCommand.java 29cce15 
>   plugins/network-elements/ovs/src/com/cloud/network/ovs/OvsTunnelManagerImpl.java b1ecaac 
>   plugins/network-elements/ovs/src/com/cloud/network/ovs/StartupOvsCommand.java b85331e 
>   plugins/network-elements/ovs/src/com/cloud/network/ovs/dao/OvsDeviceDao.java 794e45e 
>   plugins/network-elements/ovs/src/com/cloud/network/ovs/dao/OvsDeviceDaoImpl.java 11a4d48 
>   plugins/network-elements/ovs/src/com/cloud/network/ovs/dao/OvsDeviceVO.java cab63f6 
>   plugins/network-elements/ovs/src/com/cloud/network/resource/OvsResource.java a94e4f8 
>   setup/db/create-schema.sql 143023a 
> 
> Diff: https://reviews.apache.org/r/12941/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> tuna
> 
>


--===============4794415270935638188==--