cloudstack-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <>
Subject [jira] [Commented] (CLOUDSTACK-9074) Support shared networking in NiciraNVP Plugin
Date Wed, 02 Dec 2015 09:30:11 GMT


ASF GitHub Bot commented on CLOUDSTACK-9074:

Github user miguelaferreira commented on a diff in the pull request:
    --- Diff: plugins/network-elements/nicira-nvp/src/main/java/com/cloud/network/resource/wrapper/
    @@ -0,0 +1,110 @@
    +// Licensed to the Apache Software Foundation (ASF) under one
    +// or more contributor license agreements.  See the NOTICE file
    +// distributed with this work for additional information
    +// regarding copyright ownership.  The ASF licenses this file
    +// to you under the Apache License, Version 2.0 (the
    +// "License"); you may not use this file except in compliance
    +// with the License.  You may obtain a copy of the License at
    +// Unless required by applicable law or agreed to in writing,
    +// software distributed under the License is distributed on an
    +// KIND, either express or implied.  See the License for the
    +// specific language governing permissions and limitations
    +// under the License.
    +import static;
    +import java.util.ArrayList;
    +import java.util.List;
    +import org.apache.log4j.Logger;
    +@ResourceWrapper(handles =  ConfigureSharedNetworkUuidCommand.class)
    +public final class NiciraNvpConfigureSharedNetworkUuidCommandWrapper extends CommandWrapper<ConfigureSharedNetworkUuidCommand,
Answer, NiciraNvpResource>{
    +    private static final Logger s_logger = Logger.getLogger(NiciraNvpConfigureSharedNetworkUuidCommandWrapper.class);
    +    @Override
    +    public Answer execute(ConfigureSharedNetworkUuidCommand command, NiciraNvpResource
niciraNvpResource) {
    +        final String logicalRouterUuid = command.getLogicalRouterUuid();
    +        final String logicalSwitchUuid = command.getLogicalSwitchUuid();
    +        final String portIpAddress = command.getPortIpAddress();
    +        final List<NiciraNvpTag> tags = new ArrayList<NiciraNvpTag>();
    +        tags.add(new NiciraNvpTag("cs_account", command.getOwnerName()));
    +        final long networkId = command.getNetworkId();
    +        final NiciraNvpApi niciraNvpApi = niciraNvpResource.getNiciraNvpApi();
    +        s_logger.debug("Attaching Logical Switch " + logicalSwitchUuid + " on Logical
Router " + logicalRouterUuid + " for Shared Network" + networkId);
    +        //Stored for rollback
    +        LogicalRouterPort lrpi = null;
    +        LogicalSwitchPort lsp = null;
    +        try {
    +            // Create the inside port for the router
    +            lrpi = new LogicalRouterPort();
    +            lrpi.setAdminStatusEnabled(true);
    +            lrpi.setDisplayName(niciraNvpResource.truncate(networkId + "-shared-att-port",
    +            lrpi.setTags(tags);
    +            final List<String> ipAddresses = new ArrayList<String>();
    +            ipAddresses.add(portIpAddress);
    +            lrpi.setIpAddresses(ipAddresses);
    +            lrpi = niciraNvpApi.createLogicalRouterPort(logicalRouterUuid, lrpi);
    +            try {
    +                // Create the inside port on the lswitch
    +                lsp = new LogicalSwitchPort(niciraNvpResource.truncate(networkId + "-shared-att-port",
NAME_MAX_LEN), tags, true);
    +                lsp = niciraNvpApi.createLogicalSwitchPort(logicalSwitchUuid, lsp);
    +                // Attach the inside router port to the lswitch port with a PatchAttachment
    +                niciraNvpApi.updateLogicalRouterPortAttachment(logicalRouterUuid, lrpi.getUuid(),
new PatchAttachment(lsp.getUuid()));
    +                // Attach the inside lswitch port to the router with a PatchAttachment
    +                niciraNvpApi.updateLogicalSwitchPortAttachment(logicalSwitchUuid, lsp.getUuid(),
new PatchAttachment(lrpi.getUuid()));
    +            }
    +            catch (final NiciraNvpApiException e){
    +                try {
    +                    niciraNvpApi.deleteLogicalRouterPort(logicalRouterUuid, lrpi.getUuid());
    +                    if (lsp != null){
    +                        niciraNvpApi.deleteLogicalSwitchPort(logicalSwitchUuid, lsp.getUuid());
    +                    }
    +                } catch (NiciraNvpApiException ex) {
    --- End diff --
    This is indeed better. However, now you are logging it as an info message, and then making
a new exception.
    If you log something, and then throw an exception, I would say the log level should be
warn because you are delegating the handling of the exception to the callers.
    In addition, when you make a new exception out of an existing exception you should include
the original exception as the cause of the new exception.
    In this case you are using two exception as the cause `e and ex`. 
    Let us think of a way of doing this better:
    * The first exception `ex` is being handled here. So I would say, first thing is to log
it as an error (`s_logger.error("failed to do something", ex)`). This way we explain what
failed and present the cause in the logs.
    * Then we handle that exception (`ex`) and we might get a second one (`e`). 
    * This second one we don't want to handle here, and we will bubble it up wrapped in a
new exception. So then we log `e` as a warning (`s_logger.warn("failed to do something else,
namely handing the previous excpetion", e)`), and then we wrap it in  a new exception and
throw it (`throw new NiciraNvpApiException("probably the same message we set in the warn log",
    What do you think? Does this make sense to you?

> Support shared networking in NiciraNVP Plugin
> ---------------------------------------------
>                 Key: CLOUDSTACK-9074
>                 URL:
>             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

View raw message