[jira] [Commented] (CLOUDSTACK-9074) Support shared networking in NiciraNVP Plugin

["ASF GitHub Bot (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CLOUDSTACK-9074?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15033497#comment-15033497 ] 

ASF GitHub Bot commented on CLOUDSTACK-9074:
--------------------------------------------

Github user miguelaferreira commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/1094#discussion_r46263781
  
    --- Diff: api/src/com/cloud/network/Networks.java ---
    @@ -251,6 +252,10 @@ public static URI fromString(String candidate) {
                     if (com.cloud.dc.Vlan.UNTAGGED.equalsIgnoreCase(candidate)) {
                         return Native.toUri(candidate);
                     }
    +                if (UuidUtils.validateUUID(candidate)){
    +                    //Supporting lswitch uuid as vlan id: set broadcast_uri = null and add row on nicira_nvp_router_map with logicalrouter_uuid = candidate
    +                    return null;
    --- End diff --
    
    Ok, I understand the intention now. However, you could make that intention clear in the code. 
    
    You use exception handling to deal with expected behaviour because you implement the expected behaviour in the `catch`block. Given the state-of-the-practive in this project I would say that is normal, and I accept what you did.
    
    Still, if you would like to make it better, you could create a method `private boolean isVlanId(String candidate)` that has the `try...catch` in there (with the same construction you have now) but returns `true`or `false`. Then in the `fromString` method you can call the new method, and instead of having your logic in the `catch` block you will have it in a `else` block. This will make clear that the code is not handling an exceptional (error) case.
    It would look a bit like this:
    ```java
    (...)
    
    private boolean isVlanId(String candidate) {
      try {
        Long.parseLong(candidate);
        return true;
      } catch (NumberFormatException nfe) {
        return false;
      }
    }
    
    (...)
    
    if(isVlanId(candidate)) {
      return Vlan.toUri(candidate);
    } else {
      (...)
    }
    
    (...)
    ```
    
    In addition, instead of returning `null` (because that is a very bad practice, and opens up an entire category of errors, a.k.a. null dereferencing) you could return Optional<URI> and then in the method that calls `fromString` you need to handle the case of an existing URI or a non-existing URI. Optional has an [API](https://code.google.com/p/guava-libraries/wiki/UsingAndAvoidingNullExplained) for that.
    The idea behind this recommendation is that we express the fact that we might have an URI or not, in a safer and more meaningful way that returning `null`.


> Support shared networking in NiciraNVP Plugin
> ---------------------------------------------
>
>                 Key: CLOUDSTACK-9074
>                 URL: https://issues.apache.org/jira/browse/CLOUDSTACK-9074
>             Project: CloudStack
>          Issue Type: Improvement
>      Security Level: Public(Anyone can view this level - this is the default.) 
>    Affects Versions: 4.7.0
>            Reporter: Nicolas Vazquez
>             Fix For: 4.7.0
>
>
> h3. Introduction
> Currently NiciraNVP plugin supports only Isolated networking. In this mode of operations networks are assigned to individual Cloudstack accounts and on NSX side are completely isolated on the L3 level. Many use cases especially in corporate environment call for shared networking mode support. In some circumstances there also may be a need to translate shared NSX network over to a physical VLAN via L2 NSX gateway.
> Features that will be introduced to support Cloudstack shared networks in two modes of NiciraNVP plugin:
> * Shared networks mapped to a physical VLAN with L2 NSX gateway
> * Shared networks within the same L3 NSX domain. Multiple L3 NSX domains will be supported.
> h3. Features
> h4. 1) Shared networking model support
> # Support native Cloudstack shared network in NiciraNVP plugin.
> # Current code that implements isolated networking mode support will stay intact.
> # Designate network service offering by configuring VirtualNetworking provider with NiciraNVP.
> # Static/Source NAT is not used and ignored if defined in the network offering.
> # Nicira_vvp_router_map table will support non-unique logical routers to implement L3 NSX routing domains where multiple Cloudstack networks are attached to the same logical router.
> # Shared network with NSX based Virtual networking will go through the following states:
> ## Allocated
> ## Implementing
> ## Implemented
> ## Destroy
> h4. 2) Support NSX L2 gateways for L2 based VLANs mapped to a physical network
> # Optional L2gatewayserviceuuid parameter for NiciraNVP controller
> # VLAN ID of a Shared network represents VLAN to pass through L2 gateway similar to native Cloudstack shared networking
> # NSX workflow for network allocation
> ## Check if l2gatewayservice defined
> ## Create record in networks table
> ### NiciraNvpGuestNetworkGuru as Guru_name
> ### Lswitch as broadcast_doamin
> ### Vlan://vlan_id as broadcast_uri
> ## Create record in VLAN table
> # NSX workflow for network implementation
> ## Check if l2gatewayservice defined and valid
> ## Create logical switch
> ## Map logical switch to L2gateway service assigning shared network VLAN ID
> # NSX workflow for NIC management and/or hypervisor support
> ## No changes from current implementation
> h4. 3) Support NSX L3 multiple routing domains
> # VLAN ID of a Shared network represents an UUID of a NSX virtual router of a particular routing domain. We will support UUID style notation for VLAN ID. l3gatewayservice option is not used in shared networking
> # It is assumed that if connectivity to the physical networking is required then logical router is configured and connected to the physical network in advance. NiciraNVP plugin will not perform any task beyond basic connectivity to the logical router
> # Support NSX L3 multiple routing domains
> # NSX workflow for network allocation
> ## Create record in networks table
> ### NiciraNvpGuestNetworkGuru as Guru_name
> ### Lswitch as broadcast_domain
> ### NULL as broadcast_uri
> ## Create record in VLAN table
> ## Create record in nicira_nvp_router_map table
> # NSX workflow for network implementation
> ## Check if logical router exists on NSX side which UUID matches the one defined during shared network creation. This mode is activated if VLAN ID supplied in UUID style notation
> ## Create logical switch
> ## Attach logical switch to the logical router
> ## Assign shared network default gateway to the inside port of the logical router
> # NSX workflow for NIC management and/or hypervisor support
> ## No changes from current implementation
> h4. 4) API Changes
> # Existing API addNiciraNvpDevices will be updated
> ## Adding 1 new optional parameter – l2gatewayserviceuuid
> ## Adding 1 new response tag – l2gatewayserviceuuid
> # Existing API listNiciraNvpDevices will be updated
> ## Adding 1 new response tag – l2gatewayserviceuuid
> # Existing API listNics will be updated
> ## Adding 2 new optional response tag – nsxlogicalswitch, nsxlogicalswitchport



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)