ONOS-6257: fixing Region-peer-location function...
- corrected UiRegion.isRoot() implementation
- added to/from-compact-strings for LayoutLocation, so we can encode
a list of them in an annotation
- Fixed bug in DistributedRegionStore which was emiting events that
had a null region as subject.
Change-Id: I547e0c7f62385b85b191b8d63e6b569196623b84
diff --git a/core/api/src/main/java/org/onosproject/ui/model/topo/UiRegion.java b/core/api/src/main/java/org/onosproject/ui/model/topo/UiRegion.java
index ada4d15..0a2fa2f 100644
--- a/core/api/src/main/java/org/onosproject/ui/model/topo/UiRegion.java
+++ b/core/api/src/main/java/org/onosproject/ui/model/topo/UiRegion.java
@@ -123,7 +123,7 @@
* @return true if root region
*/
public boolean isRoot() {
- return id().equals(parent);
+ return parent == null;
}
/**
@@ -181,7 +181,7 @@
* @return the backing region instance
*/
public Region backingRegion() {
- return topology.services.region().getRegion(regionId);
+ return isRoot() ? null : topology.services.region().getRegion(regionId);
}
/**
diff --git a/core/api/src/main/java/org/onosproject/ui/topo/LayoutLocation.java b/core/api/src/main/java/org/onosproject/ui/topo/LayoutLocation.java
index 9e006b5..69be6aa 100644
--- a/core/api/src/main/java/org/onosproject/ui/topo/LayoutLocation.java
+++ b/core/api/src/main/java/org/onosproject/ui/topo/LayoutLocation.java
@@ -17,6 +17,12 @@
package org.onosproject.ui.topo;
+import com.google.common.base.Strings;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
import static com.google.common.base.MoreObjects.toStringHelper;
import static com.google.common.base.Preconditions.checkNotNull;
@@ -25,6 +31,14 @@
*/
public final class LayoutLocation {
private static final double ZERO_THRESHOLD = Double.MIN_VALUE * 2.0;
+ private static final String COMMA = ",";
+ private static final String TILDE = "~";
+ private static final String EMPTY = "";
+
+ private static final String E_BAD_COMPACT = "Badly formatted compact form: ";
+ private static final String E_EMPTY_ID = "id must be non-empty";
+ private static final String E_BAD_DOUBLE = "unparsable double";
+
/**
* Designates the type of location; either geographic or logical grid.
@@ -45,7 +59,6 @@
this.locType = locType;
}
-
private boolean doubleIsZero(double value) {
return value >= -ZERO_THRESHOLD && value <= ZERO_THRESHOLD;
}
@@ -70,6 +83,16 @@
}
/**
+ * Returns the location type (geo or grid), which indicates how the data
+ * is to be interpreted.
+ *
+ * @return location type
+ */
+ public Type locType() {
+ return locType;
+ }
+
+ /**
* Returns the latitude (geo) or y-coord (grid) data value.
*
* @return geo latitude or grid y-coord
@@ -87,27 +110,111 @@
return longOrX;
}
- /**
- * Returns the location type (geo or grid), which indicates how the data
- * is to be interpreted.
- *
- * @return location type
- */
- public Type locType() {
- return locType;
- }
-
@Override
public String toString() {
return toStringHelper(this)
.add("id", id)
+ .add("loc-type", locType)
.add("lat/Y", latOrY)
.add("long/X", longOrX)
- .add("loc-type", locType)
.toString();
}
/**
+ * Produces a compact string representation of this instance.
+ *
+ * @return compact string rep
+ */
+ public String toCompactListString() {
+ return id + COMMA + locType + COMMA + latOrY + COMMA + longOrX;
+ }
+
+ /**
+ * Produces a layout location instance from a compact string representation.
+ *
+ * @param s the compact string
+ * @return a corresponding instance
+ */
+ public static LayoutLocation fromCompactString(String s) {
+ String[] tokens = s.split(COMMA);
+ if (tokens.length != 4) {
+ throw new IllegalArgumentException(E_BAD_COMPACT + s);
+ }
+ String id = tokens[0];
+ String type = tokens[1];
+ String latY = tokens[2];
+ String longX = tokens[3];
+
+ if (Strings.isNullOrEmpty(id)) {
+ throw new IllegalArgumentException(E_BAD_COMPACT + E_EMPTY_ID);
+ }
+
+ double latOrY;
+ double longOrX;
+ try {
+ latOrY = Double.parseDouble(latY);
+ longOrX = Double.parseDouble(longX);
+ } catch (NumberFormatException nfe) {
+ throw new IllegalArgumentException(E_BAD_COMPACT + E_BAD_DOUBLE);
+ }
+
+ return LayoutLocation.layoutLocation(id, type, latOrY, longOrX);
+ }
+
+ /**
+ * Produces a compact encoding of a list of layout locations.
+ *
+ * @param locs array of layout location instances
+ * @return string encoding
+ */
+ public static String toCompactListString(LayoutLocation... locs) {
+ if (locs == null || locs.length == 0) {
+ return EMPTY;
+ }
+ List<LayoutLocation> lls = Arrays.asList(locs);
+ return toCompactListString(lls);
+ }
+
+ /**
+ * Produces a compact encoding of a list of layout locations.
+ *
+ * @param locs list of layout location instances
+ * @return string encoding
+ */
+ public static String toCompactListString(List<LayoutLocation> locs) {
+ // note: locs may be empty
+ if (locs == null || locs.isEmpty()) {
+ return EMPTY;
+ }
+
+ StringBuilder sb = new StringBuilder();
+ for (LayoutLocation ll : locs) {
+ sb.append(ll.toCompactListString()).append(TILDE);
+ }
+ final int len = sb.length();
+ sb.replace(len - 1, len, "");
+ return sb.toString();
+ }
+
+ /**
+ * Returns a list of layout locations from a compact string representation.
+ *
+ * @param compactList string representation
+ * @return corresponding list of layout locations
+ */
+ public static List<LayoutLocation> fromCompactListString(String compactList) {
+ List<LayoutLocation> locs = new ArrayList<>();
+ if (!Strings.isNullOrEmpty(compactList)) {
+ String[] items = compactList.split(TILDE);
+ for (String s : items) {
+ locs.add(fromCompactString(s));
+ }
+ }
+ return locs;
+ }
+
+
+ /**
* Creates an instance of a layout location.
*
* @param id an identifier for the item at this location
diff --git a/core/api/src/test/java/org/onosproject/ui/topo/LayoutLocationTest.java b/core/api/src/test/java/org/onosproject/ui/topo/LayoutLocationTest.java
index 64908fd..cca430c 100644
--- a/core/api/src/test/java/org/onosproject/ui/topo/LayoutLocationTest.java
+++ b/core/api/src/test/java/org/onosproject/ui/topo/LayoutLocationTest.java
@@ -20,9 +20,12 @@
import org.junit.Test;
import org.onosproject.ui.AbstractUiTest;
+import java.util.ArrayList;
+import java.util.List;
+
+import static com.google.common.collect.ImmutableList.of;
import static org.junit.Assert.*;
-import static org.onosproject.ui.topo.LayoutLocation.Type;
-import static org.onosproject.ui.topo.LayoutLocation.layoutLocation;
+import static org.onosproject.ui.topo.LayoutLocation.*;
/**
* Unit tests for {@link LayoutLocation}.
@@ -30,11 +33,18 @@
public class LayoutLocationTest extends AbstractUiTest {
private static final String SOME_ID = "foo";
+ private static final String OTHER_ID = "bar";
private static final double SQRT2 = 1.414;
private static final double PI = 3.142;
private static final double ZERO = 0.0;
+ private static final String COMPACT_LL_1 = "foo,GEO,3.142,1.414";
+ private static final String COMPACT_LL_2 = "bar,GRID,1.414,3.142";
+
+ private static final String COMPACT_LIST = COMPACT_LL_1 + "~" + COMPACT_LL_2;
+
private LayoutLocation ll;
+ private LayoutLocation ll2;
@Test
public void basic() {
@@ -86,4 +96,121 @@
public void nullId() {
layoutLocation(null, Type.GRID, PI, PI);
}
+
+ @Test
+ public void compactString() {
+ ll = layoutLocation(SOME_ID, Type.GEO, PI, SQRT2);
+ String s = ll.toCompactListString();
+ assertEquals("wrong compactness", COMPACT_LL_1, s);
+ }
+
+ @Test
+ public void fromCompactStringTest() {
+ ll = fromCompactString(COMPACT_LL_1);
+ verifyLL1(ll);
+ }
+
+ private void verifyLL1(LayoutLocation ll) {
+ assertEquals("LL1 bad id", SOME_ID, ll.id());
+ assertEquals("LL1 bad type", Type.GEO, ll.locType());
+ assertEquals("LL1 bad Y", PI, ll.latOrY(), TOLERANCE);
+ assertEquals("LL1 bad X", SQRT2, ll.longOrX(), TOLERANCE);
+ }
+
+ private void verifyLL2(LayoutLocation ll) {
+ assertEquals("LL1 bad id", OTHER_ID, ll.id());
+ assertEquals("LL1 bad type", Type.GRID, ll.locType());
+ assertEquals("LL1 bad Y", SQRT2, ll.latOrY(), TOLERANCE);
+ assertEquals("LL1 bad X", PI, ll.longOrX(), TOLERANCE);
+ }
+
+
+ @Test(expected = IllegalArgumentException.class)
+ public void badCompactTooShort() {
+ fromCompactString("one,two,three");
+ }
+
+ @Test(expected = IllegalArgumentException.class)
+ public void badCompactTooLong() {
+ fromCompactString("one,two,three,4,5");
+ }
+
+ @Test(expected = IllegalArgumentException.class)
+ public void badCompactNoId() {
+ fromCompactString(",GEO,1,2");
+ }
+
+ @Test(expected = IllegalArgumentException.class)
+ public void badCompactUnparsableY() {
+ fromCompactString("foo,GEO,yyy,2.3");
+ }
+
+ @Test(expected = IllegalArgumentException.class)
+ public void badCompactUnparsableX() {
+ fromCompactString("foo,GEO,0.2,xxx");
+ }
+
+ @Test
+ public void toCompactList() {
+ ll = layoutLocation(SOME_ID, Type.GEO, PI, SQRT2);
+ ll2 = layoutLocation(OTHER_ID, Type.GRID, SQRT2, PI);
+ String compact = toCompactListString(ll, ll2);
+ print(compact);
+ assertEquals("wrong list encoding", COMPACT_LIST, compact);
+ }
+
+ @Test
+ public void toCompactList2() {
+ ll = layoutLocation(SOME_ID, Type.GEO, PI, SQRT2);
+ ll2 = layoutLocation(OTHER_ID, Type.GRID, SQRT2, PI);
+ List<LayoutLocation> locs = of(ll, ll2);
+ String compact = toCompactListString(locs);
+ print(compact);
+ assertEquals("wrong list encoding", COMPACT_LIST, compact);
+ }
+
+ @Test
+ public void fromCompactList() {
+ List<LayoutLocation> locs = fromCompactListString(COMPACT_LIST);
+ ll = locs.get(0);
+ ll2 = locs.get(1);
+ verifyLL1(ll);
+ verifyLL2(ll2);
+ }
+
+ @Test
+ public void fromCompactListNull() {
+ List<LayoutLocation> locs = fromCompactListString(null);
+ assertEquals("non-empty list", 0, locs.size());
+ }
+
+ @Test
+ public void fromCompactListEmpty() {
+ List<LayoutLocation> locs = fromCompactListString("");
+ assertEquals("non-empty list", 0, locs.size());
+ }
+
+ @Test
+ public void toCompactListStringNullList() {
+ String s = toCompactListString((List<LayoutLocation>) null);
+ assertEquals("not empty string", "", s);
+ }
+
+ @Test
+ public void toCompactListStringNullArray() {
+ String s = toCompactListString((LayoutLocation[]) null);
+ assertEquals("not empty string", "", s);
+ }
+
+ @Test
+ public void toCompactListStringEmptyArray() {
+ String s = toCompactListString();
+ assertEquals("not empty string", "", s);
+ }
+
+ @Test
+ public void toCompactListStringEmptyList() {
+ String s = toCompactListString(new ArrayList<>());
+ assertEquals("not empty string", "", s);
+ }
}
diff --git a/core/net/src/main/java/org/onosproject/net/device/impl/DeviceManager.java b/core/net/src/main/java/org/onosproject/net/device/impl/DeviceManager.java
index ed26b41..a5cb416 100644
--- a/core/net/src/main/java/org/onosproject/net/device/impl/DeviceManager.java
+++ b/core/net/src/main/java/org/onosproject/net/device/impl/DeviceManager.java
@@ -931,8 +931,7 @@
}
}
- // checks if the specified device is allowed by the BasicDeviceConfig
- // and if not, removes it
+ // removes the specified device if it exists
private void kickOutBadDevice(DeviceId deviceId) {
Device badDevice = getDevice(deviceId);
if (badDevice != null) {
diff --git a/core/net/src/main/java/org/onosproject/net/region/impl/RegionManager.java b/core/net/src/main/java/org/onosproject/net/region/impl/RegionManager.java
index bf82842..f81493b 100644
--- a/core/net/src/main/java/org/onosproject/net/region/impl/RegionManager.java
+++ b/core/net/src/main/java/org/onosproject/net/region/impl/RegionManager.java
@@ -23,12 +23,15 @@
import org.apache.felix.scr.annotations.Reference;
import org.apache.felix.scr.annotations.ReferenceCardinality;
import org.apache.felix.scr.annotations.Service;
+import org.onlab.util.ItemNotFoundException;
import org.onosproject.cluster.NodeId;
import org.onosproject.event.AbstractListenerManager;
import org.onosproject.net.Annotations;
import org.onosproject.net.DefaultAnnotations;
import org.onosproject.net.DeviceId;
import org.onosproject.net.HostId;
+import org.onosproject.net.config.NetworkConfigEvent;
+import org.onosproject.net.config.NetworkConfigListener;
import org.onosproject.net.config.NetworkConfigService;
import org.onosproject.net.config.basics.BasicElementConfig;
import org.onosproject.net.config.basics.BasicRegionConfig;
@@ -40,6 +43,7 @@
import org.onosproject.net.region.RegionService;
import org.onosproject.net.region.RegionStore;
import org.onosproject.net.region.RegionStoreDelegate;
+import org.onosproject.ui.topo.LayoutLocation;
import org.slf4j.Logger;
import java.util.Collection;
@@ -51,6 +55,7 @@
import static com.google.common.collect.ImmutableList.of;
import static org.onosproject.security.AppGuard.checkPermission;
import static org.onosproject.security.AppPermission.Type.REGION_READ;
+import static org.onosproject.ui.topo.LayoutLocation.toCompactListString;
import static org.slf4j.LoggerFactory.getLogger;
/**
@@ -68,8 +73,13 @@
private static final String DEVICE_IDS_EMPTY = "Device IDs cannot be empty";
private static final String NAME_NULL = "Name cannot be null";
+ private static final String PEER_LOCATIONS = "peerLocations";
+
private final Logger log = getLogger(getClass());
+ private final NetworkConfigListener networkConfigListener =
+ new InternalNetworkConfigListener();
+
private RegionStoreDelegate delegate = this::post;
@Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
@@ -82,12 +92,14 @@
public void activate() {
store.setDelegate(delegate);
eventDispatcher.addSink(RegionEvent.class, listenerRegistry);
+ networkConfigService.addListener(networkConfigListener);
log.info("Started");
}
@Deactivate
public void deactivate() {
store.unsetDelegate(delegate);
+ networkConfigService.removeListener(networkConfigListener);
eventDispatcher.removeSink(RegionEvent.class);
log.info("Stopped");
}
@@ -99,10 +111,13 @@
private Annotations genAnnots(RegionId id) {
BasicRegionConfig cfg =
networkConfigService.getConfig(id, BasicRegionConfig.class);
-
if (cfg == null) {
return DefaultAnnotations.builder().build();
}
+ return genAnnots(cfg, id);
+ }
+
+ private Annotations genAnnots(BasicRegionConfig cfg, RegionId rid) {
DefaultAnnotations.Builder builder = DefaultAnnotations.builder()
.set(BasicElementConfig.NAME, cfg.name())
@@ -115,6 +130,9 @@
builder.set(BasicElementConfig.UI_TYPE, uiType);
}
+ List<LayoutLocation> locMappings = cfg.getMappings();
+ builder.set(PEER_LOCATIONS, toCompactListString(locMappings));
+
return builder.build();
}
@@ -126,7 +144,7 @@
checkNotNull(name, REGION_TYPE_NULL);
return store.createRegion(regionId, name, type, genAnnots(regionId),
- masterNodeIds == null ? of() : masterNodeIds);
+ masterNodeIds == null ? of() : masterNodeIds);
}
@Override
@@ -137,7 +155,7 @@
checkNotNull(name, REGION_TYPE_NULL);
return store.updateRegion(regionId, name, type, genAnnots(regionId),
- masterNodeIds == null ? of() : masterNodeIds);
+ masterNodeIds == null ? of() : masterNodeIds);
}
@Override
@@ -197,4 +215,53 @@
return ImmutableSet.of();
}
+ // by default allowed, otherwise check flag
+ private boolean isAllowed(BasicRegionConfig cfg) {
+ return (cfg == null || cfg.isAllowed());
+ }
+
+ private class InternalNetworkConfigListener implements NetworkConfigListener {
+ @Override
+ public void event(NetworkConfigEvent event) {
+ RegionId rid = (RegionId) event.subject();
+ BasicRegionConfig cfg =
+ networkConfigService.getConfig(rid, BasicRegionConfig.class);
+
+ if (!isAllowed(cfg)) {
+ kickOutBadRegion(rid);
+
+ } else {
+ // (1) Find the region
+ // (2) Syntehsize new region + cfg details
+ // (3) re-insert new region element into store
+
+ try {
+ Region region = getRegion(rid);
+ String name = region.name();
+ Region.Type type = region.type();
+ Annotations annots = genAnnots(cfg, rid);
+ List<Set<NodeId>> masterNodeIds = region.masters();
+
+ store.updateRegion(rid, name, type, annots, masterNodeIds);
+
+ } catch (ItemNotFoundException infe) {
+ log.debug("warn: no region found with id {}", rid);
+ }
+ }
+ }
+
+ @Override
+ public boolean isRelevant(NetworkConfigEvent event) {
+ return (event.type() == NetworkConfigEvent.Type.CONFIG_ADDED
+ || event.type() == NetworkConfigEvent.Type.CONFIG_UPDATED)
+ && (event.configClass().equals(BasicRegionConfig.class));
+ }
+
+ private void kickOutBadRegion(RegionId regionId) {
+ Region badRegion = getRegion(regionId);
+ if (badRegion != null) {
+ removeRegion(regionId);
+ }
+ }
+ }
}
diff --git a/core/store/dist/src/main/java/org/onosproject/store/region/impl/DistributedRegionStore.java b/core/store/dist/src/main/java/org/onosproject/store/region/impl/DistributedRegionStore.java
index e00af16..c8b5299 100644
--- a/core/store/dist/src/main/java/org/onosproject/store/region/impl/DistributedRegionStore.java
+++ b/core/store/dist/src/main/java/org/onosproject/store/region/impl/DistributedRegionStore.java
@@ -247,9 +247,12 @@
@Override
public void event(MapEvent<RegionId, Set<DeviceId>> event) {
if (event.type() != MapEvent.Type.REMOVE) {
- notifyDelegate(new RegionEvent(REGION_MEMBERSHIP_CHANGED,
- regionsById.get(event.key()),
- event.newValue().value()));
+ Region r = regionsById.get(event.key());
+ if (r != null) {
+ notifyDelegate(new RegionEvent(REGION_MEMBERSHIP_CHANGED,
+ r,
+ event.newValue().value()));
+ }
}
}
}