Committed by
Yuta HIGUCHI
Bugfix and cosmetic changes to resource subsystem
- Continuous resource was always considered not available - Support querying for child resource against Continuous (result is no children) - Resource equality to compare id and exact type - Add missing information in Continuous resource toString() - More concise String representation for ResourceId - additional logging added during above bug investigation. Change-Id: I58a95b95b91c246c3c5dbb136a1820f988c6fccd
Showing
4 changed files
with
54 additions
and
23 deletions
| ... | @@ -208,7 +208,11 @@ public abstract class Resource { | ... | @@ -208,7 +208,11 @@ public abstract class Resource { |
| 208 | if (this == obj) { | 208 | if (this == obj) { |
| 209 | return true; | 209 | return true; |
| 210 | } | 210 | } |
| 211 | - if (!(obj instanceof Resource)) { | 211 | + |
| 212 | + if (obj == null) { | ||
| 213 | + return false; | ||
| 214 | + } | ||
| 215 | + if (this.getClass() != obj.getClass()) { | ||
| 212 | return false; | 216 | return false; |
| 213 | } | 217 | } |
| 214 | final Resource that = (Resource) obj; | 218 | final Resource that = (Resource) obj; |
| ... | @@ -259,25 +263,14 @@ public abstract class Resource { | ... | @@ -259,25 +263,14 @@ public abstract class Resource { |
| 259 | 263 | ||
| 260 | @Override | 264 | @Override |
| 261 | public int hashCode() { | 265 | public int hashCode() { |
| 262 | - return Objects.hash(this.id(), this.value); | 266 | + return super.hashCode(); |
| 263 | } | 267 | } |
| 264 | 268 | ||
| 269 | + // explicitly overriding to express that we intentionally ignore | ||
| 270 | + // `value` in equality comparison | ||
| 265 | @Override | 271 | @Override |
| 266 | public boolean equals(Object obj) { | 272 | public boolean equals(Object obj) { |
| 267 | - if (this == obj) { | 273 | + return super.equals(obj); |
| 268 | - return true; | ||
| 269 | - } | ||
| 270 | - | ||
| 271 | - if (!(obj instanceof Continuous)) { | ||
| 272 | - return false; | ||
| 273 | - } | ||
| 274 | - | ||
| 275 | - if (!super.equals(obj)) { | ||
| 276 | - return false; | ||
| 277 | - } | ||
| 278 | - | ||
| 279 | - final Continuous other = (Continuous) obj; | ||
| 280 | - return Objects.equals(this.id(), other.id()); | ||
| 281 | } | 274 | } |
| 282 | 275 | ||
| 283 | /** | 276 | /** |
| ... | @@ -288,6 +281,14 @@ public abstract class Resource { | ... | @@ -288,6 +281,14 @@ public abstract class Resource { |
| 288 | public double value() { | 281 | public double value() { |
| 289 | return value; | 282 | return value; |
| 290 | } | 283 | } |
| 284 | + | ||
| 285 | + @Override | ||
| 286 | + public String toString() { | ||
| 287 | + return MoreObjects.toStringHelper(this) | ||
| 288 | + .add("id", id()) | ||
| 289 | + .add("value", value) | ||
| 290 | + .toString(); | ||
| 291 | + } | ||
| 291 | } | 292 | } |
| 292 | 293 | ||
| 293 | } | 294 | } | ... | ... |
| ... | @@ -16,7 +16,6 @@ | ... | @@ -16,7 +16,6 @@ |
| 16 | package org.onosproject.net.newresource; | 16 | package org.onosproject.net.newresource; |
| 17 | 17 | ||
| 18 | import com.google.common.annotations.Beta; | 18 | import com.google.common.annotations.Beta; |
| 19 | -import com.google.common.base.MoreObjects; | ||
| 20 | import com.google.common.collect.ImmutableList; | 19 | import com.google.common.collect.ImmutableList; |
| 21 | import org.onosproject.net.DeviceId; | 20 | import org.onosproject.net.DeviceId; |
| 22 | import org.onosproject.net.PortNumber; | 21 | import org.onosproject.net.PortNumber; |
| ... | @@ -95,8 +94,6 @@ public final class ResourceId { | ... | @@ -95,8 +94,6 @@ public final class ResourceId { |
| 95 | 94 | ||
| 96 | @Override | 95 | @Override |
| 97 | public String toString() { | 96 | public String toString() { |
| 98 | - return MoreObjects.toStringHelper(this) | 97 | + return components.toString(); |
| 99 | - .add("components", components) | ||
| 100 | - .toString(); | ||
| 101 | } | 98 | } |
| 102 | } | 99 | } | ... | ... |
| ... | @@ -34,12 +34,14 @@ import org.onosproject.net.newresource.ResourceService; | ... | @@ -34,12 +34,14 @@ import org.onosproject.net.newresource.ResourceService; |
| 34 | import org.onosproject.net.newresource.Resource; | 34 | import org.onosproject.net.newresource.Resource; |
| 35 | import org.onosproject.net.newresource.ResourceStore; | 35 | import org.onosproject.net.newresource.ResourceStore; |
| 36 | import org.onosproject.net.newresource.ResourceStoreDelegate; | 36 | import org.onosproject.net.newresource.ResourceStoreDelegate; |
| 37 | +import org.slf4j.Logger; | ||
| 37 | 38 | ||
| 38 | import java.util.Collection; | 39 | import java.util.Collection; |
| 39 | import java.util.List; | 40 | import java.util.List; |
| 40 | import java.util.stream.Collectors; | 41 | import java.util.stream.Collectors; |
| 41 | 42 | ||
| 42 | import static com.google.common.base.Preconditions.checkNotNull; | 43 | import static com.google.common.base.Preconditions.checkNotNull; |
| 44 | +import static org.slf4j.LoggerFactory.getLogger; | ||
| 43 | 45 | ||
| 44 | /** | 46 | /** |
| 45 | * An implementation of ResourceService. | 47 | * An implementation of ResourceService. |
| ... | @@ -53,18 +55,24 @@ public final class ResourceManager extends AbstractListenerManager<ResourceEvent | ... | @@ -53,18 +55,24 @@ public final class ResourceManager extends AbstractListenerManager<ResourceEvent |
| 53 | @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY) | 55 | @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY) |
| 54 | protected ResourceStore store; | 56 | protected ResourceStore store; |
| 55 | 57 | ||
| 58 | + private final Logger log = getLogger(getClass()); | ||
| 59 | + | ||
| 56 | private final ResourceStoreDelegate delegate = new InternalStoreDelegate(); | 60 | private final ResourceStoreDelegate delegate = new InternalStoreDelegate(); |
| 57 | 61 | ||
| 58 | @Activate | 62 | @Activate |
| 59 | public void activate() { | 63 | public void activate() { |
| 60 | store.setDelegate(delegate); | 64 | store.setDelegate(delegate); |
| 61 | eventDispatcher.addSink(ResourceEvent.class, listenerRegistry); | 65 | eventDispatcher.addSink(ResourceEvent.class, listenerRegistry); |
| 66 | + | ||
| 67 | + log.info("Started"); | ||
| 62 | } | 68 | } |
| 63 | 69 | ||
| 64 | @Deactivate | 70 | @Deactivate |
| 65 | public void deactivate() { | 71 | public void deactivate() { |
| 66 | store.unsetDelegate(delegate); | 72 | store.unsetDelegate(delegate); |
| 67 | eventDispatcher.removeSink(ResourceEvent.class); | 73 | eventDispatcher.removeSink(ResourceEvent.class); |
| 74 | + | ||
| 75 | + log.info("Stopped"); | ||
| 68 | } | 76 | } |
| 69 | 77 | ||
| 70 | @Override | 78 | @Override | ... | ... |
| ... | @@ -143,6 +143,9 @@ public class ConsistentResourceStore extends AbstractStore<ResourceEvent, Resour | ... | @@ -143,6 +143,9 @@ public class ConsistentResourceStore extends AbstractStore<ResourceEvent, Resour |
| 143 | @Override | 143 | @Override |
| 144 | public boolean register(List<Resource> resources) { | 144 | public boolean register(List<Resource> resources) { |
| 145 | checkNotNull(resources); | 145 | checkNotNull(resources); |
| 146 | + if (log.isTraceEnabled()) { | ||
| 147 | + resources.forEach(r -> log.trace("registering {}", r)); | ||
| 148 | + } | ||
| 146 | 149 | ||
| 147 | TransactionContext tx = service.transactionContextBuilder().build(); | 150 | TransactionContext tx = service.transactionContextBuilder().build(); |
| 148 | tx.begin(); | 151 | tx.begin(); |
| ... | @@ -324,17 +327,36 @@ public class ConsistentResourceStore extends AbstractStore<ResourceEvent, Resour | ... | @@ -324,17 +327,36 @@ public class ConsistentResourceStore extends AbstractStore<ResourceEvent, Resour |
| 324 | checkNotNull(resource); | 327 | checkNotNull(resource); |
| 325 | checkArgument(resource instanceof Resource.Discrete || resource instanceof Resource.Continuous); | 328 | checkArgument(resource instanceof Resource.Discrete || resource instanceof Resource.Continuous); |
| 326 | 329 | ||
| 330 | + // check if it's registered or not. | ||
| 331 | + Versioned<Set<Resource>> v = childMap.get(resource.parent().get()); | ||
| 332 | + if (v == null || !v.value().contains(resource)) { | ||
| 333 | + return false; | ||
| 334 | + } | ||
| 335 | + | ||
| 327 | if (resource instanceof Resource.Discrete) { | 336 | if (resource instanceof Resource.Discrete) { |
| 337 | + // check if already consumed | ||
| 328 | return getConsumer((Resource.Discrete) resource).isEmpty(); | 338 | return getConsumer((Resource.Discrete) resource).isEmpty(); |
| 329 | } else { | 339 | } else { |
| 330 | - return isAvailable((Resource.Continuous) resource); | 340 | + Resource.Continuous requested = (Resource.Continuous) resource; |
| 341 | + Resource.Continuous registered = v.value().stream() | ||
| 342 | + .filter(c -> c.equals(resource)) | ||
| 343 | + .findFirst() | ||
| 344 | + .map(c -> (Resource.Continuous) c) | ||
| 345 | + .get(); | ||
| 346 | + if (registered.value() < requested.value()) { | ||
| 347 | + // Capacity < requested, can never satisfy | ||
| 348 | + return false; | ||
| 349 | + } | ||
| 350 | + // check if there's enough left | ||
| 351 | + return isAvailable(requested); | ||
| 331 | } | 352 | } |
| 332 | } | 353 | } |
| 333 | 354 | ||
| 334 | private boolean isAvailable(Resource.Continuous resource) { | 355 | private boolean isAvailable(Resource.Continuous resource) { |
| 335 | Versioned<ContinuousResourceAllocation> allocation = continuousConsumers.get(resource.id()); | 356 | Versioned<ContinuousResourceAllocation> allocation = continuousConsumers.get(resource.id()); |
| 336 | if (allocation == null) { | 357 | if (allocation == null) { |
| 337 | - return false; | 358 | + // no allocation (=no consumer) full registered resources available |
| 359 | + return true; | ||
| 338 | } | 360 | } |
| 339 | 361 | ||
| 340 | return hasEnoughResource(allocation.value().original(), resource, allocation.value()); | 362 | return hasEnoughResource(allocation.value().original(), resource, allocation.value()); |
| ... | @@ -362,7 +384,10 @@ public class ConsistentResourceStore extends AbstractStore<ResourceEvent, Resour | ... | @@ -362,7 +384,10 @@ public class ConsistentResourceStore extends AbstractStore<ResourceEvent, Resour |
| 362 | @Override | 384 | @Override |
| 363 | public Collection<Resource> getChildResources(Resource parent) { | 385 | public Collection<Resource> getChildResources(Resource parent) { |
| 364 | checkNotNull(parent); | 386 | checkNotNull(parent); |
| 365 | - checkArgument(parent instanceof Resource.Discrete); | 387 | + if (!(parent instanceof Resource.Discrete)) { |
| 388 | + // only Discrete resource can have child resource | ||
| 389 | + return ImmutableList.of(); | ||
| 390 | + } | ||
| 366 | 391 | ||
| 367 | Versioned<Set<Resource>> children = childMap.get((Resource.Discrete) parent); | 392 | Versioned<Set<Resource>> children = childMap.get((Resource.Discrete) parent); |
| 368 | if (children == null) { | 393 | if (children == null) { | ... | ... |
-
Please register or login to post a comment