Add optional "name" parameter in interface configuration.

Interfaces can now be added and deleted by name. Interfaces without names
cannot be updated or deleted.

Change-Id: Icb2188b1c9abf3017724f751a93457920a53ba03

@@ -17,6 +17,7 @@
 package org.onosproject.cli.net;
 
 import com.google.common.collect.Sets;
+import org.apache.karaf.shell.commands.Argument;
 import org.apache.karaf.shell.commands.Command;
 import org.apache.karaf.shell.commands.Option;
 import org.onlab.packet.MacAddress;
@@ -36,11 +37,15 @@ import java.util.Set;
         description = "Adds a new configured interface")
 public class InterfaceAddCommand extends AbstractShellCommand {
 
-    @Option(name = "-c", aliases = "--connectPoint",
+    @Argument(index = 0, name = "port",
             description = "Device port that the interface is associated with",
             required = true, multiValued = false)
     private String connectPoint = null;
 
+    @Argument(index = 1, name = "name", description = "Interface name",
+            required = true, multiValued = false)
+    private String name = null;
+
     @Option(name = "-m", aliases = "--mac",
             description = "MAC address of the interface",
             required = false, multiValued = false)
@@ -72,10 +77,13 @@ public class InterfaceAddCommand extends AbstractShellCommand {
 
         VlanId vlanId = vlan == null ? VlanId.NONE : VlanId.vlanId(Short.parseShort(vlan));
 
-        Interface intf = new Interface(ConnectPoint.deviceConnectPoint(connectPoint),
+        Interface intf = new Interface(name,
+                ConnectPoint.deviceConnectPoint(connectPoint),
                 ipAddresses, macAddr, vlanId);
 
         interfaceService.add(intf);
+
+        print("Interface added");
     }
 
 }

@@ -18,7 +18,6 @@ package org.onosproject.cli.net;
 
 import org.apache.karaf.shell.commands.Argument;
 import org.apache.karaf.shell.commands.Command;
-import org.onlab.packet.VlanId;
 import org.onosproject.cli.AbstractShellCommand;
 import org.onosproject.incubator.net.intf.InterfaceAdminService;
 import org.onosproject.net.ConnectPoint;
@@ -35,17 +34,23 @@ public class InterfaceRemoveCommand extends AbstractShellCommand {
             required = true, multiValued = false)
     private String connectPoint = null;
 
-    @Argument(index = 1, name = "vlan",
-            description = "Interface vlan",
+    @Argument(index = 1, name = "name",
+            description = "Interface name",
             required = true, multiValued = false)
-    private String vlan = null;
+    private String name = null;
 
     @Override
     protected void execute() {
         InterfaceAdminService interfaceService = get(InterfaceAdminService.class);
 
-        interfaceService.remove(ConnectPoint.deviceConnectPoint(connectPoint),
-                VlanId.vlanId(Short.parseShort(vlan)));
+        boolean success = interfaceService.remove(
+                ConnectPoint.deviceConnectPoint(connectPoint), name);
+
+        if (success) {
+            print("Interface removed");
+        } else {
+            print("Unable to remove interface");
+        }
     }
 
 }

@@ -35,6 +35,9 @@ public class InterfacesListCommand extends AbstractShellCommand {
     private static final String FORMAT =
             "port=%s/%s, ips=%s, mac=%s, vlan=%s";
 
+    private static final String NAME_FORMAT =
+            "%s: port=%s/%s, ips=%s, mac=%s, vlan=%s";
+
     @Override
     protected void execute() {
         InterfaceService interfaceService = get(InterfaceService.class);
@@ -44,8 +47,14 @@ public class InterfacesListCommand extends AbstractShellCommand {
         Collections.sort(interfaces, Comparators.INTERFACES_COMPARATOR);
 
         for (Interface intf : interfaces) {
-            print(FORMAT, intf.connectPoint().deviceId(), intf.connectPoint().port(),
-                    intf.ipAddresses(), intf.mac(), intf.vlan());
+            if (intf.name().equals(Interface.NO_INTERFACE_NAME)) {
+                print(FORMAT, intf.connectPoint().deviceId(), intf.connectPoint().port(),
+                        intf.ipAddresses(), intf.mac(), intf.vlan());
+            } else {
+                print(NAME_FORMAT, intf.name(), intf.connectPoint().deviceId(),
+                        intf.connectPoint().port(), intf.ipAddresses(),
+                        intf.mac(), intf.vlan());
+            }
         }
     }
 

@@ -358,15 +358,16 @@
         </command>
         <command>
             <action class="org.onosproject.cli.net.InterfaceAddCommand"/>
-            <optional-completers>
-                <entry key="-c" value-ref="connectPointCompleter"/>
-                <entry key="--connectPoint" value-ref="connectPointCompleter"/>
-            </optional-completers>
+            <completers>
+                <ref component-id="connectPointCompleter" />
+                <ref component-id="placeholderCompleter" />
+            </completers>
         </command>
         <command>
             <action class="org.onosproject.cli.net.InterfaceRemoveCommand"/>
             <completers>
                 <ref component-id="connectPointCompleter"/>
+                <ref component-id="placeholderCompleter" />
             </completers>
         </command>
         <command>
@@ -505,4 +506,6 @@
     <bean id="upDownCompleter" class="org.onosproject.cli.UpDownCompleter"/>
     <bean id="encapTypeCompleter" class="org.onosproject.cli.net.EncapTypeCompleter"/>
 
+    <bean id="placeholderCompleter" class="org.onosproject.cli.PlaceholderCompleter"/>
+
 </blueprint>

@@ -28,6 +28,7 @@ import org.onosproject.net.ConnectPoint;
 import org.onosproject.net.config.Config;
 import org.onosproject.net.host.InterfaceIpAddress;
 
+import java.util.Iterator;
 import java.util.Set;
 
 /**
@@ -35,6 +36,7 @@ import java.util.Set;
  */
 @Beta
 public class InterfaceConfig extends Config<ConnectPoint> {
+    public static final String NAME = "name";
     public static final String IPS = "ips";
     public static final String MAC = "mac";
     public static final String VLAN = "vlan";
@@ -52,6 +54,8 @@ public class InterfaceConfig extends Config<ConnectPoint> {
 
         try {
             for (JsonNode intfNode : array) {
+                String name = intfNode.path(NAME).asText(null);
+
                 Set<InterfaceIpAddress> ips = getIps(intfNode);
 
                 String mac = intfNode.path(MAC).asText();
@@ -59,7 +63,7 @@ public class InterfaceConfig extends Config<ConnectPoint> {
 
                 VlanId vlan = getVlan(intfNode);
 
-                interfaces.add(new Interface(subject, ips, macAddr, vlan));
+                interfaces.add(new Interface(name, subject, ips, macAddr, vlan));
             }
         } catch (IllegalArgumentException e) {
             throw new ConfigException(CONFIG_VALUE_ERROR, e);
@@ -76,6 +80,8 @@ public class InterfaceConfig extends Config<ConnectPoint> {
     public void addInterface(Interface intf) {
         ObjectNode intfNode = array.addObject();
 
+        intfNode.put(NAME, intf.name());
+
         if (intf.mac() != null) {
             intfNode.put(MAC, intf.mac().toString());
         }
@@ -92,12 +98,14 @@ public class InterfaceConfig extends Config<ConnectPoint> {
     /**
      * Removes an interface from the config.
      *
-     * @param intf interface to remove
+     * @param name name of the interface to remove
      */
-    public void removeInterface(Interface intf) {
-        for (int i = 0; i < array.size(); i++) {
-            if (intf.vlan().equals(getVlan(node))) {
-                array.remove(i);
+    public void removeInterface(String name) {
+        Iterator<JsonNode> it = array.iterator();
+        while (it.hasNext()) {
+            JsonNode node = it.next();
+            if (node.path(NAME).asText().equals(name)) {
+                it.remove();
                 break;
             }
         }

@@ -30,11 +30,13 @@ import static com.google.common.base.Preconditions.checkNotNull;
 
 /**
  * An Interface maps network configuration information (such as addresses and
- * vlans) to a port in the network. This is considered a L2/L3 network
- * interface.
+ * vlans) to a port in the network.
  */
 @Beta
 public class Interface {
+    public static final String NO_INTERFACE_NAME = "";
+
+    private final String name;
     private final ConnectPoint connectPoint;
     private final Set<InterfaceIpAddress> ipAddresses;
     private final MacAddress macAddress;
@@ -43,14 +45,16 @@ public class Interface {
     /**
      * Creates new Interface with the provided configuration.
      *
+     * @param name name of the interface
      * @param connectPoint the connect point this interface maps to
      * @param ipAddresses Set of IP addresses
      * @param macAddress MAC address
      * @param vlan VLAN ID
      */
-    public Interface(ConnectPoint connectPoint,
+    public Interface(String name, ConnectPoint connectPoint,
                      Set<InterfaceIpAddress> ipAddresses,
                      MacAddress macAddress, VlanId vlan) {
+        this.name = name == null ? NO_INTERFACE_NAME : name;
         this.connectPoint = checkNotNull(connectPoint);
         this.ipAddresses = ipAddresses == null ? Sets.newHashSet() : ipAddresses;
         this.macAddress = macAddress == null ? MacAddress.NONE : macAddress;
@@ -58,6 +62,29 @@ public class Interface {
     }
 
     /**
+     * Creates new Interface with the provided configuration.
+     *
+     * @param connectPoint the connect point this interface maps to
+     * @param ipAddresses Set of IP addresses
+     * @param macAddress MAC address
+     * @param vlan VLAN ID
+     */
+    public Interface(ConnectPoint connectPoint,
+                     Set<InterfaceIpAddress> ipAddresses,
+                     MacAddress macAddress, VlanId vlan) {
+        this(NO_INTERFACE_NAME, connectPoint, ipAddresses, macAddress, vlan);
+    }
+
+    /**
+     * Retrieves the name of the interface.
+     *
+     * @return name
+     */
+    public String name() {
+        return name;
+    }
+
+    /**
      * Retrieves the connection point that this interface maps to.
      *
      * @return the connection point
@@ -101,7 +128,8 @@ public class Interface {
 
         Interface otherInterface = (Interface) other;
 
-        return Objects.equals(connectPoint, otherInterface.connectPoint) &&
+        return Objects.equals(name, otherInterface.name) &&
+                Objects.equals(connectPoint, otherInterface.connectPoint) &&
                 Objects.equals(ipAddresses, otherInterface.ipAddresses) &&
                 Objects.equals(macAddress, otherInterface.macAddress) &&
                 Objects.equals(vlan, otherInterface.vlan);
@@ -109,12 +137,13 @@ public class Interface {
 
     @Override
     public int hashCode() {
-        return Objects.hash(connectPoint, ipAddresses, macAddress, vlan);
+        return Objects.hash(connectPoint, name, ipAddresses, macAddress, vlan);
     }
 
     @Override
     public String toString() {
         return MoreObjects.toStringHelper(getClass())
+                .add("name", name)
                 .add("connectPoint", connectPoint)
                 .add("ipAddresses", ipAddresses)
                 .add("macAddress", macAddress)

@@ -16,13 +16,13 @@
 
 package org.onosproject.incubator.net.intf;
 
-import org.onlab.packet.VlanId;
 import org.onosproject.net.ConnectPoint;
 
 /**
  * Provides a means to modify the interfaces configuration.
  */
 public interface InterfaceAdminService {
+
     /**
      * Adds a new interface configuration to the system.
      *
@@ -34,7 +34,7 @@ public interface InterfaceAdminService {
      * Removes an interface configuration from the system.
      *
      * @param connectPoint connect point of the interface
-     * @param vlanId vlan id
+     * @param name name of the interface
      */
-    void remove(ConnectPoint connectPoint, VlanId vlanId);
+    boolean remove(ConnectPoint connectPoint, String name);
 }

@@ -18,6 +18,7 @@ package org.onosproject.incubator.net.intf.impl;
 
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Maps;
+import com.google.common.collect.Sets;
 import org.apache.felix.scr.annotations.Activate;
 import org.apache.felix.scr.annotations.Component;
 import org.apache.felix.scr.annotations.Deactivate;
@@ -39,9 +40,11 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.util.Collections;
+import java.util.Iterator;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 import static java.util.stream.Collectors.collectingAndThen;
 import static java.util.stream.Collectors.toSet;
@@ -145,7 +148,7 @@ public class InterfaceManager implements InterfaceService,
 
     private void updateInterfaces(InterfaceConfig intfConfig) {
         try {
-            interfaces.put(intfConfig.subject(), intfConfig.getInterfaces());
+            interfaces.put(intfConfig.subject(), Sets.newHashSet(intfConfig.getInterfaces()));
         } catch (ConfigException e) {
             log.error("Error in interface config", e);
         }
@@ -157,18 +160,7 @@ public class InterfaceManager implements InterfaceService,
 
     @Override
     public void add(Interface intf) {
-        if (interfaces.containsKey(intf.connectPoint())) {
-            boolean conflict = interfaces.get(intf.connectPoint()).stream()
-                    .filter(i -> i.connectPoint().equals(intf.connectPoint()))
-                    .filter(i -> i.mac().equals(intf.mac()))
-                    .filter(i -> i.vlan().equals(intf.vlan()))
-                    .findAny().isPresent();
-
-            if (conflict) {
-                log.error("Can't add interface because it conflicts with existing config");
-                return;
-            }
-        }
+        addInternal(intf);
 
         InterfaceConfig config =
                 configService.addConfig(intf.connectPoint(), CONFIG_CLASS);
@@ -178,29 +170,72 @@ public class InterfaceManager implements InterfaceService,
         configService.applyConfig(intf.connectPoint(), CONFIG_CLASS, config.node());
     }
 
+    private void addInternal(Interface intf) {
+        interfaces.compute(intf.connectPoint(), (cp, current) -> {
+            if (current == null) {
+                return Sets.newHashSet(intf);
+            }
+
+            Iterator<Interface> it = current.iterator();
+            while (it.hasNext()) {
+                Interface i = it.next();
+                if (i.name().equals(intf.name())) {
+                    it.remove();
+                    break;
+                }
+            }
+
+            current.add(intf);
+            return current;
+        });
+    }
+
     @Override
-    public void remove(ConnectPoint connectPoint, VlanId vlanId) {
-        Optional<Interface> intf = interfaces.get(connectPoint).stream()
-                .filter(i -> i.vlan().equals(vlanId))
-                .findAny();
-
-        if (!intf.isPresent()) {
-            log.error("Can't find interface {}/{} to remove", connectPoint, vlanId);
-            return;
-        }
+    public boolean remove(ConnectPoint connectPoint, String name) {
+        boolean success = removeInternal(name, connectPoint);
 
-        InterfaceConfig config = configService.addConfig(intf.get().connectPoint(), CONFIG_CLASS);
-        config.removeInterface(intf.get());
+        InterfaceConfig config = configService.addConfig(connectPoint, CONFIG_CLASS);
+        config.removeInterface(name);
 
         try {
             if (config.getInterfaces().isEmpty()) {
                 configService.removeConfig(connectPoint, CONFIG_CLASS);
             } else {
-                configService.applyConfig(intf.get().connectPoint(), CONFIG_CLASS, config.node());
+                configService.applyConfig(connectPoint, CONFIG_CLASS, config.node());
             }
         } catch (ConfigException e) {
             log.error("Error reading interfaces JSON", e);
         }
+
+        return success;
+    }
+
+    public boolean removeInternal(String name, ConnectPoint connectPoint) {
+        AtomicBoolean removed = new AtomicBoolean(false);
+
+        interfaces.compute(connectPoint, (cp, current) -> {
+            if (current == null) {
+                return null;
+            }
+
+            Iterator<Interface> it = current.iterator();
+            while (it.hasNext()) {
+                Interface i = it.next();
+                if (i.name().equals(name)) {
+                    it.remove();
+                    removed.set(true);
+                    break;
+                }
+            }
+
+            if (current.isEmpty()) {
+                return null;
+            } else {
+                return current;
+            }
+        });
+
+        return removed.get();
     }
 
     /**