Fixes on Contrail and Mon InMemory plugins; adding comments about the changes.

Signed-off-by: Hugo Trippaers <htrippaers@schubergphilis.com>

plugins/event-bus/inmemory/src/org/apache/cloudstack/mom/inmemory/InMemoryEventBus.java

@@ -26,33 +26,36 @@ import java.util.concurrent.ConcurrentHashMap;
 import javax.ejb.Local;
 import javax.naming.ConfigurationException;
 
-import com.cloud.utils.Pair;
-import org.apache.log4j.Logger;
-
 import org.apache.cloudstack.framework.events.Event;
 import org.apache.cloudstack.framework.events.EventBus;
 import org.apache.cloudstack.framework.events.EventBusException;
 import org.apache.cloudstack.framework.events.EventSubscriber;
 import org.apache.cloudstack.framework.events.EventTopic;
+import org.apache.log4j.Logger;
 
+import com.cloud.utils.Pair;
 import com.cloud.utils.component.ManagerBase;
 
 @Local(value = EventBus.class)
 public class InMemoryEventBus extends ManagerBase implements EventBus {
 
-    private String name;
     private static final Logger s_logger = Logger.getLogger(InMemoryEventBus.class);
-    private static ConcurrentHashMap<UUID, Pair<EventTopic, EventSubscriber>> s_subscribers;
+
+    private final static ConcurrentHashMap<UUID, Pair<EventTopic, EventSubscriber>> subscribers;
+
+    static {
+        subscribers = new ConcurrentHashMap<UUID, Pair<EventTopic, EventSubscriber>>();
+    }
 
     @Override
     public boolean configure(String name, Map<String, Object> params) throws ConfigurationException {
-        s_subscribers = new ConcurrentHashMap<UUID, Pair<EventTopic, EventSubscriber>>();
+        _name = name;
         return true;
     }
 
     @Override
     public void setName(String name) {
-        this.name = name;
+        _name = name;
     }
 
     @Override
@@ -62,29 +65,35 @@ public class InMemoryEventBus extends ManagerBase implements EventBus {
         }
         UUID subscriberId = UUID.randomUUID();
 
-        s_subscribers.put(subscriberId, new Pair(topic, subscriber));
+        subscribers.put(subscriberId, new Pair<EventTopic, EventSubscriber>(topic, subscriber));
         return subscriberId;
     }
 
     @Override
     public void unsubscribe(UUID subscriberId, EventSubscriber subscriber) throws EventBusException {
-        if (s_subscribers != null && s_subscribers.isEmpty()) {
+        if (subscriberId == null) {
+            throw new EventBusException("Cannot unregister a null subscriberId.");
+        }
+
+        if (subscribers.isEmpty()) {
             throw new EventBusException("There are no registered subscribers to unregister.");
         }
-        if (s_subscribers.get(subscriberId) == null) {
+
+        if (!subscribers.containsKey(subscriberId)) {
             throw new EventBusException("No subscriber found with subscriber id " + subscriberId);
+        } else {
+            subscribers.remove(subscriberId);
         }
-        s_subscribers.remove(subscriberId);
     }
 
     @Override
     public void publish(Event event) throws EventBusException {
-        if (s_subscribers == null || s_subscribers.isEmpty()) {
+        if (subscribers == null || subscribers.isEmpty()) {
             return; // no subscriber to publish to, so just return
         }
 
-        for (UUID subscriberId : s_subscribers.keySet()) {
+        for (UUID subscriberId : subscribers.keySet()) {
-            Pair<EventTopic, EventSubscriber>  subscriberDetails =  s_subscribers.get(subscriberId);
+            Pair<EventTopic, EventSubscriber>  subscriberDetails =  subscribers.get(subscriberId);
             // if the event matches subscribers interested event topic then call back the subscriber with the event
             if (isEventMatchesTopic(event, subscriberDetails.first())) {
                 EventSubscriber subscriber =  subscriberDetails.second();
@@ -108,6 +117,10 @@ public class InMemoryEventBus extends ManagerBase implements EventBus {
         return true;
     }
 
+    public int totalSubscribers() {
+        return subscribers.size();
+    }
+
     private String replaceNullWithWildcard(String key) {
         if (key == null || key.isEmpty()) {
             return "*";
@@ -122,7 +135,7 @@ public class InMemoryEventBus extends ManagerBase implements EventBus {
         eventTopicSource = eventTopicSource.replace(".", "-");
         String eventSource = replaceNullWithWildcard(event.getEventSource());
         eventSource = eventSource.replace(".", "-");
-        if (eventTopicSource != "*" && eventSource != "*" && !eventTopicSource.equalsIgnoreCase(eventSource)) {
+        if (!eventTopicSource.equals("*") && !eventSource.equals("*") && !eventTopicSource.equalsIgnoreCase(eventSource)) {
             return false;
         }
 
@@ -130,7 +143,7 @@ public class InMemoryEventBus extends ManagerBase implements EventBus {
         eventTopicCategory = eventTopicCategory.replace(".", "-");
         String eventCategory = replaceNullWithWildcard(event.getEventCategory());
         eventCategory = eventCategory.replace(".", "-");
-        if (eventTopicCategory != "*" && eventCategory != "*" && !eventTopicCategory.equalsIgnoreCase(eventCategory)) {
+        if (!eventTopicCategory.equals("*") && !eventCategory.equals("*") && !eventTopicCategory.equalsIgnoreCase(eventCategory)) {
             return false;
         }
 
@@ -138,7 +151,7 @@ public class InMemoryEventBus extends ManagerBase implements EventBus {
         eventTopicType = eventTopicType.replace(".", "-");
         String eventType = replaceNullWithWildcard(event.getEventType());
         eventType = eventType.replace(".", "-");
-        if (eventTopicType != "*" && eventType != "*" && !eventTopicType.equalsIgnoreCase(eventType)) {
+        if (!eventTopicType.equals("*") && !eventType.equals("*") && !eventTopicType.equalsIgnoreCase(eventType)) {
             return false;
         }
 
@@ -146,7 +159,7 @@ public class InMemoryEventBus extends ManagerBase implements EventBus {
         eventTopicResourceType = eventTopicResourceType.replace(".", "-");
         String resourceType = replaceNullWithWildcard(event.getResourceType());
         resourceType = resourceType.replace(".", "-");
-        if (eventTopicResourceType != "*" && resourceType != "*" && !eventTopicResourceType.equalsIgnoreCase(resourceType)) {
+        if (!eventTopicResourceType.equals("*") && !resourceType.equals("*") && !eventTopicResourceType.equalsIgnoreCase(resourceType)) {
             return false;
         }
 
@@ -154,7 +167,7 @@ public class InMemoryEventBus extends ManagerBase implements EventBus {
         resourceUuid = resourceUuid.replace(".", "-");
         String eventTopicresourceUuid = replaceNullWithWildcard(topic.getResourceUUID());
         eventTopicresourceUuid = eventTopicresourceUuid.replace(".", "-");
-        if (resourceUuid != "*" && eventTopicresourceUuid != "*" && !resourceUuid.equalsIgnoreCase(eventTopicresourceUuid)) {
+        if (!resourceUuid.equals("*") && !eventTopicresourceUuid.equals("*") && !resourceUuid.equalsIgnoreCase(eventTopicresourceUuid)) {
             return false;
         }
 

plugins/event-bus/inmemory/test/org/apache/cloudstack/mom/inmemory/InMemoryEventBusTest.java

@@ -0,0 +1,162 @@
+/*
+ * 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
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.cloudstack.mom.inmemory;
+
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+
+import java.util.UUID;
+
+import org.apache.cloudstack.framework.events.Event;
+import org.apache.cloudstack.framework.events.EventBusException;
+import org.apache.cloudstack.framework.events.EventSubscriber;
+import org.apache.cloudstack.framework.events.EventTopic;
+import org.junit.Test;
+
+import com.cloud.utils.UuidUtils;
+
+public class InMemoryEventBusTest {
+
+    @Test
+    public void testConfigure() throws Exception {
+        String name = "test001";
+
+        InMemoryEventBus bus = new InMemoryEventBus();
+        boolean success = bus.configure(name, null);
+
+        assertTrue(success);
+        assertTrue(name.equals(bus.getName()));
+    }
+
+    @Test
+    public void testSubscribe() throws Exception {
+        EventTopic topic = mock(EventTopic.class);
+        EventSubscriber subscriber = mock(EventSubscriber.class);
+
+        InMemoryEventBus bus = new InMemoryEventBus();
+
+        UUID uuid = bus.subscribe(topic, subscriber);
+        assertNotNull(uuid);
+
+        String uuidStr = uuid.toString();
+        assertTrue(UuidUtils.validateUUID(uuidStr));
+        assertTrue(bus.totalSubscribers() == 1);
+
+        bus.unsubscribe(uuid, subscriber);
+        assertTrue(bus.totalSubscribers() == 0);
+    }
+
+    @Test(expected = EventBusException.class)
+    public void testSubscribeFailTopic() throws Exception {
+        EventSubscriber subscriber = mock(EventSubscriber.class);
+
+        InMemoryEventBus bus = new InMemoryEventBus();
+
+        bus.subscribe(null, subscriber);
+    }
+
+    @Test(expected = EventBusException.class)
+    public void testSubscribeFailSubscriber() throws Exception {
+        EventTopic topic = mock(EventTopic.class);
+
+        InMemoryEventBus bus = new InMemoryEventBus();
+
+        bus.subscribe(topic, null);
+    }
+
+    @Test
+    public void testUnsubscribe() throws Exception {
+        EventTopic topic = mock(EventTopic.class);
+        EventSubscriber subscriber = mock(EventSubscriber.class);
+
+        InMemoryEventBus bus = new InMemoryEventBus();
+
+        UUID uuid = bus.subscribe(topic, subscriber);
+        assertNotNull(uuid);
+
+        String uuidStr = uuid.toString();
+
+        assertTrue(UuidUtils.validateUUID(uuidStr));
+        assertTrue(bus.totalSubscribers() == 1);
+        //
+        bus.unsubscribe(uuid, subscriber);
+        assertTrue(bus.totalSubscribers() == 0);
+    }
+
+    @Test(expected = EventBusException.class)
+    public void testUnsubscribeFailEmpty() throws Exception {
+        UUID uuid = UUID.randomUUID();
+
+        InMemoryEventBus bus = new InMemoryEventBus();
+        bus.unsubscribe(uuid, null);
+    }
+
+    @Test(expected = EventBusException.class)
+    public void testUnsubscribeFailNull() throws Exception {
+        InMemoryEventBus bus = new InMemoryEventBus();
+        bus.unsubscribe(null, null);
+    }
+
+    @Test(expected = EventBusException.class)
+    public void testUnsubscribeFailWrongUUID() throws Exception {
+        EventSubscriber subscriber = mock(EventSubscriber.class);
+        InMemoryEventBus bus = new InMemoryEventBus();
+        UUID uuid = UUID.randomUUID();
+
+        bus.unsubscribe(uuid, subscriber);
+    }
+
+    @Test
+    public void testPublish() throws Exception {
+        EventTopic topic = mock(EventTopic.class);
+        EventSubscriber subscriber = mock(EventSubscriber.class);
+        Event event = mock(Event.class);
+
+        InMemoryEventBus bus = new InMemoryEventBus();
+
+        UUID uuid = bus.subscribe(topic, subscriber);
+        assertNotNull(uuid);
+
+        String uuidStr = uuid.toString();
+        assertTrue(UuidUtils.validateUUID(uuidStr));
+        assertTrue(bus.totalSubscribers() == 1);
+
+        bus.publish(event);
+
+        verify(subscriber, times(1)).onEvent(event);
+
+        bus.unsubscribe(uuid, subscriber);
+        assertTrue(bus.totalSubscribers() == 0);
+    }
+
+    @Test
+    public void testPublishEmpty() throws Exception {
+        EventSubscriber subscriber = mock(EventSubscriber.class);
+        Event event = mock(Event.class);
+
+        InMemoryEventBus bus = new InMemoryEventBus();
+        bus.publish(event);
+
+        verify(subscriber, times(0)).onEvent(event);
+    }
+}

plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/management/ContrailManagerImpl.java

@@ -31,9 +31,9 @@ import java.util.Set;
 import java.util.Timer;
 import java.util.TimerTask;
 
+import javax.ejb.Local;
 import javax.inject.Inject;
 import javax.naming.ConfigurationException;
-import javax.ejb.Local;
 
 import net.juniper.contrail.api.ApiConnector;
 import net.juniper.contrail.api.ApiConnectorFactory;
@@ -44,19 +44,16 @@ import net.juniper.contrail.api.types.FloatingIpPool;
 import net.juniper.contrail.api.types.NetworkPolicy;
 import net.juniper.contrail.api.types.VirtualNetwork;
 
-import org.apache.commons.lang.StringUtils;
-import org.apache.log4j.Logger;
-
-import com.google.common.collect.ImmutableList;
-
 import org.apache.cloudstack.network.contrail.model.FloatingIpModel;
 import org.apache.cloudstack.network.contrail.model.FloatingIpPoolModel;
 import org.apache.cloudstack.network.contrail.model.ModelController;
 import org.apache.cloudstack.network.contrail.model.VirtualNetworkModel;
+import org.apache.commons.io.IOUtils;
+import org.apache.commons.lang.StringUtils;
+import org.apache.log4j.Logger;
 
 import com.cloud.configuration.ConfigurationManager;
 import com.cloud.configuration.ConfigurationService;
-import com.cloud.server.ConfigurationServer;
 import com.cloud.dc.DataCenter;
 import com.cloud.dc.dao.DataCenterDao;
 import com.cloud.dc.dao.VlanDao;
@@ -75,20 +72,21 @@ import com.cloud.network.dao.NetworkVO;
 import com.cloud.network.dao.PhysicalNetworkDao;
 import com.cloud.network.dao.PhysicalNetworkServiceProviderDao;
 import com.cloud.network.dao.PhysicalNetworkVO;
-import com.cloud.offering.NetworkOffering.Availability;
+import com.cloud.network.vpc.NetworkACLVO;
-import com.cloud.offering.NetworkOffering;
-import com.cloud.offerings.NetworkOfferingVO;
-import com.cloud.offerings.dao.NetworkOfferingDao;
-import com.cloud.projects.ProjectVO;
-import com.cloud.network.vpc.dao.NetworkACLDao;
-import com.cloud.network.vpc.dao.VpcDao;
-import com.cloud.network.vpc.dao.VpcOfferingDao;
 import com.cloud.network.vpc.VpcOffering;
 import com.cloud.network.vpc.VpcOfferingVO;
 import com.cloud.network.vpc.VpcProvisioningService;
 import com.cloud.network.vpc.VpcVO;
-import com.cloud.network.vpc.NetworkACLVO;
+import com.cloud.network.vpc.dao.NetworkACLDao;
+import com.cloud.network.vpc.dao.VpcDao;
+import com.cloud.network.vpc.dao.VpcOfferingDao;
+import com.cloud.offering.NetworkOffering;
+import com.cloud.offering.NetworkOffering.Availability;
+import com.cloud.offerings.NetworkOfferingVO;
+import com.cloud.offerings.dao.NetworkOfferingDao;
+import com.cloud.projects.ProjectVO;
 import com.cloud.projects.dao.ProjectDao;
+import com.cloud.server.ConfigurationServer;
 import com.cloud.user.Account;
 import com.cloud.user.dao.AccountDao;
 import com.cloud.utils.PropertiesUtil;
@@ -100,6 +98,7 @@ import com.cloud.vm.VMInstanceVO;
 import com.cloud.vm.dao.NicDao;
 import com.cloud.vm.dao.UserVmDao;
 import com.cloud.vm.dao.VMInstanceDao;
+import com.google.common.collect.ImmutableList;
 
 @Local(value = { ContrailManager.class})
 public class ContrailManagerImpl extends ManagerBase implements ContrailManager {
@@ -159,7 +158,7 @@ public class ContrailManagerImpl extends ManagerBase implements ContrailManager
     private Timer _dbSyncTimer;
     private int _dbSyncInterval = DB_SYNC_INTERVAL_DEFAULT;
     private final String configuration = "contrail.properties";
-    private ModelDatabase _database;
+    private final ModelDatabase _database;
     private ModelController _controller;
 
     ContrailManagerImpl() {
@@ -300,6 +299,7 @@ public class ContrailManagerImpl extends ManagerBase implements ContrailManager
     public boolean configure(String name, Map<String, Object> params) throws ConfigurationException {
 
         File configFile = PropertiesUtil.findConfigFile(configuration);
+        FileInputStream fileStream = null;
         try {
             String hostname = null;
             int port = 0;
@@ -307,10 +307,12 @@ public class ContrailManagerImpl extends ManagerBase implements ContrailManager
                 return false;
             } else {
                 final Properties configProps = new Properties();
-                configProps.load(new FileInputStream(configFile));
+                fileStream = new FileInputStream(configFile);
+                configProps.load(fileStream);
+
                 String value = configProps.getProperty("management.db_sync_interval");
                 if (value != null) {
-                    _dbSyncInterval = Integer.valueOf(value);
+                    _dbSyncInterval = Integer.parseInt(value);
                 }
                 hostname = configProps.getProperty("api.hostname");
                 String portStr = configProps.getProperty("api.port");
@@ -326,6 +328,8 @@ public class ContrailManagerImpl extends ManagerBase implements ContrailManager
             s_logger.debug("Exception in configure: " + ex);
             ex.printStackTrace();
             throw new ConfigurationException();
+        } finally {
+            IOUtils.closeQuietly(fileStream);
         }
 
         _controller = new ModelController(this, _api, _vmDao, _networksDao, _nicDao, _vlanDao, _ipAddressDao);

plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/model/ModelObject.java

@@ -39,8 +39,16 @@ import com.cloud.exception.InternalErrorException;
  * The update method pushes updates to the contrail API server.
  */
 public interface ModelObject {
+
     public static class ModelReference implements Comparable<ModelReference>, Serializable {
-        WeakReference<ModelObject> reference;
+
+        private static final long serialVersionUID = -2019113974956703526L;
+
+        /*
+         * WeakReference class is not serializable by definition. So, we cannot enforce its serialization unless we write the implementation of
+         * methods writeObject() and readObject(). Since the code was already not serializing it, it's been marked as transient.
+         */
+        transient WeakReference<ModelObject> reference;
 
         ModelReference(ModelObject obj) {
             reference = new WeakReference<ModelObject>(obj);

plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/model/ServiceInstanceModel.java

@@ -30,11 +30,10 @@ import net.juniper.contrail.api.types.ServiceInstanceType;
 import net.juniper.contrail.api.types.ServiceTemplate;
 import net.juniper.contrail.api.types.ServiceTemplateType;
 
+import org.apache.cloudstack.network.contrail.management.ContrailManager;
 import org.apache.commons.lang.StringUtils;
 import org.apache.log4j.Logger;
 
-import org.apache.cloudstack.network.contrail.management.ContrailManager;
-
 import com.cloud.offering.ServiceOffering;
 import com.cloud.template.VirtualMachineTemplate;
 import com.cloud.utils.exception.CloudRuntimeException;
@@ -72,8 +71,14 @@ public class ServiceInstanceModel extends ModelObjectBase {
         String parent_name;
         if (project != null) {
             parent_name = StringUtils.join(project.getQualifiedName(), ':');
+
+            _projectId = project.getUuid();
         } else {
             parent_name = ContrailManager.VNC_ROOT_DOMAIN + ":" + ContrailManager.VNC_DEFAULT_PROJECT;
+
+            //In the original code, if the projectId is null, it will simply throw NPE on the last line (nr. 90) of the method where the projectId.getUuid() is called.
+            //This was added as a way to avoid NPE. Should we perhaps throw a CloudRuntimeException if the project object is null?
+            _projectId = UUID.randomUUID().toString();
         }
         _fqName = parent_name + ":" + name;
 
@@ -86,8 +91,6 @@ public class ServiceInstanceModel extends ModelObjectBase {
         _templateName = template.getName();
         _templateId = template.getUuid();
         _templateUrl = template.getUrl();
-
-        _projectId = project.getUuid();
     }
 
     /**

plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/model/VirtualMachineModel.java

@@ -18,6 +18,7 @@
 package org.apache.cloudstack.network.contrail.model;
 
 import java.io.IOException;
+import java.text.MessageFormat;
 import java.util.List;
 import java.util.Map;
 import java.util.TreeSet;
@@ -28,13 +29,14 @@ import net.juniper.contrail.api.types.ServiceInstance;
 import net.juniper.contrail.api.types.VirtualMachine;
 
 import org.apache.cloudstack.network.contrail.management.ContrailManager;
-import org.apache.log4j.Logger;
 import org.apache.commons.lang.StringUtils;
+import org.apache.log4j.Logger;
 
 import com.cloud.exception.InternalErrorException;
 import com.cloud.network.dao.NetworkDao;
 import com.cloud.network.dao.NetworkVO;
 import com.cloud.uservm.UserVm;
+import com.cloud.utils.UuidUtils;
 import com.cloud.utils.exception.CloudRuntimeException;
 import com.cloud.vm.NicVO;
 import com.cloud.vm.VMInstanceVO;
@@ -83,10 +85,26 @@ public class VirtualMachineModel extends ModelObjectBase {
             final Gson json = new Gson();
             Map<String, String> kvmap = json.fromJson(userVm.getUserData(), new TypeToken<Map<String, String>>() {
             }.getType());
-            String data = kvmap.get("service-instance");
+            //Renamed "data" to "serviceUuid" because it's clearer.
-            if (data != null) {
+            String serviceUuid = kvmap.get("service-instance");
+            if (serviceUuid != null) {
+                /*
+                 * UUID.fromString() does not validate an UUID properly. I tried, for example, informing less digits in the UUID, where 12 were expected,
+                 * and the UUID.fromstring() did not thrown the exception as expected. However, if you try UUID.fromString("aaa") it breaks, but if you try
+                 * UUID.fromString("3dd4fa6e-2899-4429-b818-d34fe8df5") it doesn't (the last portion should have 12, instead of 9 digits).
+                 *
+                 * In other fix I added the validate UUID method to the UuidUtil classes.
+                 */
+                if (UuidUtils.validateUUID(serviceUuid)) {
                     /* link the object with the service instance */
-                buildServiceInstance(controller, data);
+                    buildServiceInstance(controller, serviceUuid);
+                } else {
+                    // Throw a CloudRuntimeException in case the UUID is not valid.
+                    String message = "Invalid UUID ({0}) given for the service-instance for VM {1}.";
+                    message = MessageFormat.format(message, instance.getId(), serviceUuid);
+                    s_logger.warn(message);
+                    throw new CloudRuntimeException(message);
+                }
             }
         }
     }
@@ -109,22 +127,20 @@ public class VirtualMachineModel extends ModelObjectBase {
             s_logger.warn("service-instance read", ex);
             throw new CloudRuntimeException("Unable to read service-instance object", ex);
         }
+
         ServiceInstanceModel siModel;
-        if (siObj == null) {
-            siModel = new ServiceInstanceModel(serviceUuid);
-            siModel.build(controller, siObj);
-            manager.getDatabase().getServiceInstances().add(siModel);
-        } else {
         String fqn = StringUtils.join(siObj.getQualifiedName(), ':');
         siModel = manager.getDatabase().lookupServiceInstance(fqn);
         if (siModel == null) {
-                if (siObj == null) {
             siModel = new ServiceInstanceModel(serviceUuid);
             siModel.build(controller, siObj);
             manager.getDatabase().getServiceInstances().add(siModel);
         }
-            }
+        /*
-        }
+         * The code that was under the ELSE was never executed and due to that has been removed.
+         * Also, in the case siObj was null, it was going pass it as parameter to the build() method in the
+         * siModel object.
+         */
         _serviceModel = siModel;
     }
 

plugins/network-elements/juniper-contrail/src/org/apache/cloudstack/network/contrail/model/VirtualNetworkModel.java

@@ -32,9 +32,8 @@ import net.juniper.contrail.api.types.VirtualNetwork;
 import net.juniper.contrail.api.types.VirtualNetworkPolicyType;
 import net.juniper.contrail.api.types.VnSubnetsType;
 
-import org.apache.log4j.Logger;
-
 import org.apache.cloudstack.network.contrail.management.ContrailManager;
+import org.apache.log4j.Logger;
 
 import com.cloud.dc.VlanVO;
 import com.cloud.dc.dao.VlanDao;
@@ -49,7 +48,7 @@ public class VirtualNetworkModel extends ModelObjectBase {
 
     private String _uuid;
     private long _id;
-    private TrafficType _trafficType;
+    private final TrafficType _trafficType;
 
     /*
      * current state for object properties
@@ -389,7 +388,7 @@ public class VirtualNetworkModel extends ModelObjectBase {
             dbSubnets.add(vlan.getVlanGateway() + cidr);
         }
     } else {
-            dbSubnets.add(this._gateway + this._prefix);
+        dbSubnets.add(_gateway + _prefix);
     }
 
     List<ObjectReference<VnSubnetsType>> ipamRefs = _vn.getNetworkIpam();
@@ -416,7 +415,7 @@ public class VirtualNetworkModel extends ModelObjectBase {
     diff.removeAll(vncSubnets);
 
     if (!diff.isEmpty()) {
-            s_logger.debug("Subnets changed, network: " + this._name + "; db: " + dbSubnets + ", vnc: " + vncSubnets + ", diff: " + diff);
+        s_logger.debug("Subnets changed, network: " + _name + "; db: " + dbSubnets + ", vnc: " + vncSubnets + ", diff: " + diff);
         return false;
     }
 
@@ -447,7 +446,7 @@ public class VirtualNetworkModel extends ModelObjectBase {
     @Override
     public boolean compare(ModelController controller, ModelObject o) {
         VirtualNetworkModel latest;
-        assert this._vn != null : "vnc virtual network current is not initialized";
+        assert _vn != null : "vnc virtual network current is not initialized";
 
         try {
             latest = (VirtualNetworkModel)o;
@@ -464,7 +463,7 @@ public class VirtualNetworkModel extends ModelObjectBase {
         }
         assert latest._vn != null : "vnc virtual network new is not initialized";
 
-        List<ObjectReference<VnSubnetsType>> currentIpamRefs = this._vn.getNetworkIpam();
+        List<ObjectReference<VnSubnetsType>> currentIpamRefs = _vn.getNetworkIpam();
         List<ObjectReference<VnSubnetsType>> newIpamRefs = latest._vn.getNetworkIpam();
         List<String> currentSubnets = new ArrayList<String>();
         List<String> newSubnets = new ArrayList<String>();
@@ -503,42 +502,60 @@ public class VirtualNetworkModel extends ModelObjectBase {
         diff.removeAll(newSubnets);
 
         if (!diff.isEmpty()) {
-            s_logger.debug("Subnets differ, network: " + this._name + "; db: " + currentSubnets + ", vnc: " + newSubnets + ", diff: " + diff);
+            s_logger.debug("Subnets differ, network: " + _name + "; db: " + currentSubnets + ", vnc: " + newSubnets + ", diff: " + diff);
             return false;
         }
 
-        List<ObjectReference<VirtualNetworkPolicyType>> currentPolicyRefs = this._vn.getNetworkPolicy();
+        List<ObjectReference<VirtualNetworkPolicyType>> currentPolicyRefs = _vn.getNetworkPolicy();
         List<ObjectReference<VirtualNetworkPolicyType>> latestPolicyRefs = latest._vn.getNetworkPolicy();
 
         if (currentPolicyRefs == null && latestPolicyRefs == null) {
             return true;
         }
 
-        if ((currentPolicyRefs == null && latestPolicyRefs != null) ||
+        if ((currentPolicyRefs == null && latestPolicyRefs != null) || (currentPolicyRefs != null
-                (currentPolicyRefs != null && latestPolicyRefs == null) ||
+                && latestPolicyRefs == null)) {
-                (currentPolicyRefs.size() != latestPolicyRefs.size())) {
             return false;
         }
 
-        if (currentPolicyRefs.isEmpty() && latestPolicyRefs.isEmpty()) {
+        if ((currentPolicyRefs != null && latestPolicyRefs != null) && (currentPolicyRefs.size() != latestPolicyRefs.size())) {
+            return false;
+        }
+
+        if ((currentPolicyRefs != null && latestPolicyRefs != null) &&  currentPolicyRefs.isEmpty()
+                && latestPolicyRefs.isEmpty()) {
             return true;
         }
 
         //both must be non empty lists
-        ObjectReference<VirtualNetworkPolicyType> ref1 = currentPolicyRefs.get(0);
+        ObjectReference<VirtualNetworkPolicyType> ref1 = null;
-        ObjectReference<VirtualNetworkPolicyType> ref2 = latestPolicyRefs.get(0);
+
+        if (currentPolicyRefs != null) {
+            ref1 = currentPolicyRefs.get(0);
+        }
+
+        ObjectReference<VirtualNetworkPolicyType> ref2 = null;
+
+        if (latestPolicyRefs != null) {
+            ref2 = latestPolicyRefs.get(0);
+        }
+
+        if (ref1 == null && ref2 == null) {
+            return true;
+        }
 
         if ((ref1 != null && ref2 == null) || (ref1 == null && ref2 != null)) {
             return false;
         }
 
-        if ((ref1.getUuid() != null && ref2.getUuid() == null) || (ref1.getUuid() == null && ref2.getUuid() != null)) {
+        if ((ref1 != null && ref2 != null) && ((ref1.getUuid() != null && ref2.getUuid() == null)
+                || (ref1.getUuid() == null && ref2.getUuid() != null))) {
             return false;
         }
-        if (ref1.getUuid() == null && ref2.getUuid() == null) {
+        if ((ref1 != null && ref2 != null) && (ref1.getUuid() == null && ref2.getUuid() == null)) {
             return true;
         }
-        if (!ref1.getUuid().equals(ref2.getUuid())) {
+        if ((ref1 != null && ref2 != null) && !ref1.getUuid().equals(ref2.getUuid())) {
             return false;
         }
         return true;

plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/model/VirtualMachineModelTest.java

@@ -28,11 +28,10 @@ import junit.framework.TestCase;
 import net.juniper.contrail.api.ApiConnector;
 import net.juniper.contrail.api.ApiConnectorMock;
 
-import org.apache.log4j.Logger;
-import org.junit.Test;
-
 import org.apache.cloudstack.network.contrail.management.ContrailManagerImpl;
 import org.apache.cloudstack.network.contrail.management.ModelDatabase;
+import org.apache.log4j.Logger;
+import org.junit.Test;
 
 import com.cloud.network.Network;
 import com.cloud.network.dao.NetworkVO;
@@ -118,7 +117,5 @@ public class VirtualMachineModelTest extends TestCase {
 
         //verify
         assertTrue(vmModel.verify(controller));
-
     }
-
 }

plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/model/VirtualNetworkModelTest.java

@@ -21,26 +21,138 @@ import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
 import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
 import java.util.UUID;
 
 import junit.framework.TestCase;
 import net.juniper.contrail.api.ApiConnector;
 import net.juniper.contrail.api.ApiConnectorMock;
-
+import net.juniper.contrail.api.ObjectReference;
-import org.apache.log4j.Logger;
+import net.juniper.contrail.api.types.VirtualNetwork;
-import org.junit.Test;
+import net.juniper.contrail.api.types.VirtualNetworkPolicyType;
 
 import org.apache.cloudstack.network.contrail.management.ContrailManager;
 import org.apache.cloudstack.network.contrail.management.ContrailManagerImpl;
 import org.apache.cloudstack.network.contrail.management.ModelDatabase;
+import org.apache.log4j.Logger;
+import org.junit.Before;
+import org.junit.Test;
 
+import com.cloud.dc.dao.VlanDao;
 import com.cloud.network.Network.State;
 import com.cloud.network.Networks.TrafficType;
 import com.cloud.network.dao.NetworkVO;
 
 public class VirtualNetworkModelTest extends TestCase {
+
     private static final Logger s_logger = Logger.getLogger(VirtualNetworkModelTest.class);
 
+    private ModelController controller;
+
+    private VirtualNetworkModel vnModel;
+    private VirtualNetworkModel vnModel1;
+    private VirtualNetworkModel vnModel2;
+    private VirtualNetworkModel vnModel3;
+
+    @Override
+    @Before
+    public void setUp() throws IOException {
+        //Network UUIDs
+        String uuid = UUID.randomUUID().toString();
+        String uuid1 = UUID.randomUUID().toString();
+        String uuid2 = UUID.randomUUID().toString();
+        String uuid3 = UUID.randomUUID().toString();
+
+        //ContrailManager
+        ContrailManagerImpl contrailMgr = mock(ContrailManagerImpl.class);
+
+        controller = mock(ModelController.class);
+        VlanDao vlanDao = mock(VlanDao.class);
+
+        ApiConnector api = mock(ApiConnectorMock.class);
+
+        //Mock classes/methods
+        when(controller.getManager()).thenReturn(contrailMgr);
+        when(controller.getApiAccessor()).thenReturn(api);
+        when(controller.getVlanDao()).thenReturn(vlanDao);
+
+        //Policy References used by vnModel1
+        List<ObjectReference<VirtualNetworkPolicyType>> policyRefs1 = new ArrayList<ObjectReference<VirtualNetworkPolicyType>>();
+        ObjectReference<VirtualNetworkPolicyType> objectReference1 = new ObjectReference<VirtualNetworkPolicyType>();
+        policyRefs1.add(objectReference1);
+
+        //Policy References used by vnModel2
+        List<ObjectReference<VirtualNetworkPolicyType>> policyRefs2 = new ArrayList<ObjectReference<VirtualNetworkPolicyType>>();
+        ObjectReference<VirtualNetworkPolicyType> objectReference2 = new ObjectReference<VirtualNetworkPolicyType>();
+        policyRefs2.add(objectReference2);
+
+        //Policy References used by vnModel3
+        List<ObjectReference<VirtualNetworkPolicyType>> policyRefs3 = new ArrayList<ObjectReference<VirtualNetworkPolicyType>>();
+        ObjectReference<VirtualNetworkPolicyType> objectReference3 = new ObjectReference<VirtualNetworkPolicyType>();
+        objectReference3.setReference(Arrays.asList(""), null, null, UUID.randomUUID().toString());
+
+        policyRefs3.add(objectReference3);
+
+        //Network to be compared with
+        VirtualNetwork vn = mock(VirtualNetwork.class);
+        when(api.findById(VirtualNetwork.class, uuid)).thenReturn(vn);
+
+        //Network to be compared with
+        VirtualNetwork vn1 = mock(VirtualNetwork.class);
+        when(api.findById(VirtualNetwork.class, uuid1)).thenReturn(vn1);
+        when(vn1.getNetworkPolicy()).thenReturn(policyRefs1);
+
+        //Network to be compared to
+        VirtualNetwork vn2 = mock(VirtualNetwork.class);
+        when(api.findById(VirtualNetwork.class, uuid2)).thenReturn(vn2);
+        when(vn2.getNetworkPolicy()).thenReturn(policyRefs2);
+
+        //Network to be compared to
+        VirtualNetwork vn3 = mock(VirtualNetwork.class);
+        when(api.findById(VirtualNetwork.class, uuid3)).thenReturn(vn3);
+        when(vn3.getNetworkPolicy()).thenReturn(policyRefs3);
+
+        //Virtual-Network 1
+        NetworkVO network1 = mock(NetworkVO.class);
+        when(network1.getName()).thenReturn("testnetwork");
+        when(network1.getState()).thenReturn(State.Allocated);
+        when(network1.getGateway()).thenReturn("10.1.1.1");
+        when(network1.getCidr()).thenReturn("10.1.1.0/24");
+        when(network1.getPhysicalNetworkId()).thenReturn(42L);
+        when(network1.getDomainId()).thenReturn(10L);
+        when(network1.getAccountId()).thenReturn(42L);
+
+        //Virtual-Network 2
+        NetworkVO network2 = mock(NetworkVO.class);
+        when(network2.getName()).thenReturn("Testnetwork");
+        when(network2.getState()).thenReturn(State.Allocated);
+        when(network2.getGateway()).thenReturn("10.1.1.1");
+        when(network2.getCidr()).thenReturn("10.1.1.0/24");
+        when(network2.getPhysicalNetworkId()).thenReturn(42L);
+        when(network2.getDomainId()).thenReturn(10L);
+        when(network2.getAccountId()).thenReturn(42L);
+
+        //Virtual-Network 3
+        NetworkVO network3 = mock(NetworkVO.class);
+        when(network3.getName()).thenReturn("Testnetwork");
+        when(network3.getState()).thenReturn(State.Allocated);
+        when(network3.getGateway()).thenReturn("10.1.1.1");
+        when(network3.getCidr()).thenReturn("10.1.1.0/24");
+        when(network3.getPhysicalNetworkId()).thenReturn(42L);
+        when(network3.getDomainId()).thenReturn(10L);
+        when(network3.getAccountId()).thenReturn(42L);
+
+        when(contrailMgr.getCanonicalName(network1)).thenReturn("testnetwork");
+        when(contrailMgr.getProjectId(network1.getDomainId(), network1.getAccountId())).thenReturn("testProjectId");
+
+        vnModel = new VirtualNetworkModel(network1, uuid, "testnetwork", TrafficType.Guest);
+        vnModel1 = new VirtualNetworkModel(network1, uuid1, "testnetwork", TrafficType.Guest);
+        vnModel2 = new VirtualNetworkModel(network2, uuid2, "testnetwork", TrafficType.Guest);
+        vnModel3 = new VirtualNetworkModel(network3, uuid3, "testnetwork", TrafficType.Guest);
+    }
+
     @Test
     public void testDBLookup() {
         ModelDatabase db = new ModelDatabase();
@@ -99,4 +211,27 @@ public class VirtualNetworkModelTest extends TestCase {
         assertTrue(vnModel.verify(controller));
     }
 
+    @Test
+    public void testCompareDifferentVirtualNetwork() throws IOException {
+        //This one returns false because one network has Policy References
+        vnModel.read(controller);
+
+        assertFalse(vnModel.compare(controller, vnModel1));
+    }
+
+    @Test
+    public void testCompareSameVirtualNetwork() throws IOException {
+        //This one returns true because both networks have the same Policy References
+        vnModel1.read(controller);
+
+        assertTrue(vnModel1.compare(controller, vnModel2));
+    }
+
+    @Test
+    public void testCompareDifferentDeeperVirtualNetwork() throws IOException {
+        //This one returns false because one network has Policy References
+        vnModel2.read(controller);
+
+        assertFalse(vnModel2.compare(controller, vnModel3));
+    }
 }

utils/src/com/cloud/utils/UuidUtils.java

@@ -16,8 +16,16 @@
 // under the License.
 package com.cloud.utils;
 
+import org.apache.xerces.impl.xpath.regex.RegularExpression;
+
 public class UuidUtils {
+
     public final static String first(String uuid) {
         return uuid.substring(0, uuid.indexOf('-'));
     }
+
+    public static boolean validateUUID(String uuid) {
+        RegularExpression regex = new RegularExpression("[0-9a-fA-F]{8}(?:-[0-9a-fA-F]{4}){3}-[0-9a-fA-F]{12}");
+        return regex.matches(uuid);
+    }
 }

utils/test/com/cloud/utils/UuidUtilsTest.java

@@ -0,0 +1,23 @@
+package com.cloud.utils;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import org.junit.Test;
+
+public class UuidUtilsTest {
+
+    @Test
+    public void testValidateUUIDPass() throws Exception {
+        String serviceUuid = "f81a9aa3-1f7d-466f-b04b-f2b101486bae";
+
+        assertTrue(UuidUtils.validateUUID(serviceUuid));
+    }
+
+    @Test
+    public void testValidateUUIDFail() throws Exception {
+        String serviceUuid = "6fc6ce7-d503-4f95-9e68-c9cd281b13df";
+
+        assertFalse(UuidUtils.validateUUID(serviceUuid));
+    }
+}