Added error count for IntentData (ONOS-1060)
Change-Id: Ida6313603c15fb6c1c1793c298206587b370a13e
Showing
6 changed files
with
61 additions
and
4 deletions
| ... | @@ -44,6 +44,7 @@ public class IntentData { //FIXME need to make this "immutable" | ... | @@ -44,6 +44,7 @@ public class IntentData { //FIXME need to make this "immutable" |
| 44 | private IntentState state; | 44 | private IntentState state; |
| 45 | private Timestamp version; | 45 | private Timestamp version; |
| 46 | private NodeId origin; | 46 | private NodeId origin; |
| 47 | + private int errorCount; | ||
| 47 | 48 | ||
| 48 | private List<Intent> installables; | 49 | private List<Intent> installables; |
| 49 | 50 | ||
| ... | @@ -75,6 +76,7 @@ public class IntentData { //FIXME need to make this "immutable" | ... | @@ -75,6 +76,7 @@ public class IntentData { //FIXME need to make this "immutable" |
| 75 | version = intentData.version; | 76 | version = intentData.version; |
| 76 | origin = intentData.origin; | 77 | origin = intentData.origin; |
| 77 | installables = intentData.installables; | 78 | installables = intentData.installables; |
| 79 | + errorCount = intentData.errorCount; | ||
| 78 | } | 80 | } |
| 79 | 81 | ||
| 80 | // kryo constructor | 82 | // kryo constructor |
| ... | @@ -165,6 +167,32 @@ public class IntentData { //FIXME need to make this "immutable" | ... | @@ -165,6 +167,32 @@ public class IntentData { //FIXME need to make this "immutable" |
| 165 | } | 167 | } |
| 166 | 168 | ||
| 167 | /** | 169 | /** |
| 170 | + * Increments the error count for this intent. | ||
| 171 | + */ | ||
| 172 | + public void incrementErrorCount() { | ||
| 173 | + errorCount++; | ||
| 174 | + } | ||
| 175 | + | ||
| 176 | + /** | ||
| 177 | + * Sets the error count for this intent. | ||
| 178 | + * | ||
| 179 | + * @param newCount new count | ||
| 180 | + */ | ||
| 181 | + public void setErrorCount(int newCount) { | ||
| 182 | + errorCount = newCount; | ||
| 183 | + } | ||
| 184 | + | ||
| 185 | + /** | ||
| 186 | + * Returns the number of times that this intent has encountered an error | ||
| 187 | + * during installation or withdrawal. | ||
| 188 | + * | ||
| 189 | + * @return error count | ||
| 190 | + */ | ||
| 191 | + public int errorCount() { | ||
| 192 | + return errorCount; | ||
| 193 | + } | ||
| 194 | + | ||
| 195 | + /** | ||
| 168 | * Sets the intent installables to the given list of intents. | 196 | * Sets the intent installables to the given list of intents. |
| 169 | * | 197 | * |
| 170 | * @param installables list of installables for this intent | 198 | * @param installables list of installables for this intent | ... | ... |
| ... | @@ -59,12 +59,17 @@ public class IntentCleanup implements Runnable, IntentListener { | ... | @@ -59,12 +59,17 @@ public class IntentCleanup implements Runnable, IntentListener { |
| 59 | private static final Logger log = getLogger(IntentManager.class); | 59 | private static final Logger log = getLogger(IntentManager.class); |
| 60 | 60 | ||
| 61 | private static final int DEFAULT_PERIOD = 5; //seconds | 61 | private static final int DEFAULT_PERIOD = 5; //seconds |
| 62 | + private static final int DEFAULT_THRESHOLD = 5; //tries | ||
| 62 | 63 | ||
| 63 | @Property(name = "period", intValue = DEFAULT_PERIOD, | 64 | @Property(name = "period", intValue = DEFAULT_PERIOD, |
| 64 | label = "Frequency in ms between cleanup runs") | 65 | label = "Frequency in ms between cleanup runs") |
| 65 | protected int period = DEFAULT_PERIOD; | 66 | protected int period = DEFAULT_PERIOD; |
| 66 | private long periodMs; | 67 | private long periodMs; |
| 67 | 68 | ||
| 69 | + @Property(name = "retryThreshold", intValue = DEFAULT_THRESHOLD, | ||
| 70 | + label = "Number of times to retry CORRUPT intent without delay") | ||
| 71 | + private int retryThreshold = DEFAULT_THRESHOLD; | ||
| 72 | + | ||
| 68 | @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY) | 73 | @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY) |
| 69 | protected IntentService service; | 74 | protected IntentService service; |
| 70 | 75 | ||
| ... | @@ -106,6 +111,9 @@ public class IntentCleanup implements Runnable, IntentListener { | ... | @@ -106,6 +111,9 @@ public class IntentCleanup implements Runnable, IntentListener { |
| 106 | try { | 111 | try { |
| 107 | String s = get(properties, "period"); | 112 | String s = get(properties, "period"); |
| 108 | newPeriod = isNullOrEmpty(s) ? period : Integer.parseInt(s.trim()); | 113 | newPeriod = isNullOrEmpty(s) ? period : Integer.parseInt(s.trim()); |
| 114 | + | ||
| 115 | + s = get(properties, "retryThreshold"); | ||
| 116 | + retryThreshold = isNullOrEmpty(s) ? period : Integer.parseInt(s.trim()); | ||
| 109 | } catch (NumberFormatException e) { | 117 | } catch (NumberFormatException e) { |
| 110 | log.warn(e.getMessage()); | 118 | log.warn(e.getMessage()); |
| 111 | newPeriod = period; | 119 | newPeriod = period; |
| ... | @@ -147,9 +155,9 @@ public class IntentCleanup implements Runnable, IntentListener { | ... | @@ -147,9 +155,9 @@ public class IntentCleanup implements Runnable, IntentListener { |
| 147 | } | 155 | } |
| 148 | 156 | ||
| 149 | private void resubmitCorrupt(IntentData intentData, boolean checkThreshold) { | 157 | private void resubmitCorrupt(IntentData intentData, boolean checkThreshold) { |
| 150 | - //TODO we might want to give up when retry count exceeds a threshold | 158 | + if (checkThreshold && intentData.errorCount() >= retryThreshold) { |
| 151 | - // FIXME drop this if we exceed retry threshold | 159 | + return; // threshold met or exceeded |
| 152 | - | 160 | + } |
| 153 | 161 | ||
| 154 | switch (intentData.request()) { | 162 | switch (intentData.request()) { |
| 155 | case INSTALL_REQ: | 163 | case INSTALL_REQ: |
| ... | @@ -211,7 +219,7 @@ public class IntentCleanup implements Runnable, IntentListener { | ... | @@ -211,7 +219,7 @@ public class IntentCleanup implements Runnable, IntentListener { |
| 211 | 219 | ||
| 212 | @Override | 220 | @Override |
| 213 | public void event(IntentEvent event) { | 221 | public void event(IntentEvent event) { |
| 214 | - // fast path for CORRUPT intents, retry on event notification | 222 | + // this is the fast path for CORRUPT intents, retry on event notification. |
| 215 | //TODO we might consider using the timer to back off for subsequent retries | 223 | //TODO we might consider using the timer to back off for subsequent retries |
| 216 | if (event.type() == IntentEvent.Type.CORRUPT) { | 224 | if (event.type() == IntentEvent.Type.CORRUPT) { |
| 217 | Key key = event.subject().key(); | 225 | Key key = event.subject().key(); | ... | ... |
| ... | @@ -448,6 +448,7 @@ public class IntentManager | ... | @@ -448,6 +448,7 @@ public class IntentManager |
| 448 | log.warn("Failed installation: {} {} on {}", | 448 | log.warn("Failed installation: {} {} on {}", |
| 449 | installData.key(), installData.intent(), ops); | 449 | installData.key(), installData.intent(), ops); |
| 450 | installData.setState(CORRUPT); | 450 | installData.setState(CORRUPT); |
| 451 | + installData.incrementErrorCount(); | ||
| 451 | store.write(installData); | 452 | store.write(installData); |
| 452 | } | 453 | } |
| 453 | // if toUninstall was cause of error, then CORRUPT (another job will clean this up) | 454 | // if toUninstall was cause of error, then CORRUPT (another job will clean this up) |
| ... | @@ -456,6 +457,7 @@ public class IntentManager | ... | @@ -456,6 +457,7 @@ public class IntentManager |
| 456 | log.warn("Failed withdrawal: {} {} on {}", | 457 | log.warn("Failed withdrawal: {} {} on {}", |
| 457 | uninstallData.key(), uninstallData.intent(), ops); | 458 | uninstallData.key(), uninstallData.intent(), ops); |
| 458 | uninstallData.setState(CORRUPT); | 459 | uninstallData.setState(CORRUPT); |
| 460 | + uninstallData.incrementErrorCount(); | ||
| 459 | store.write(uninstallData); | 461 | store.write(uninstallData); |
| 460 | } | 462 | } |
| 461 | } | 463 | } | ... | ... |
| ... | @@ -21,6 +21,7 @@ import org.onosproject.net.intent.impl.IntentProcessor; | ... | @@ -21,6 +21,7 @@ import org.onosproject.net.intent.impl.IntentProcessor; |
| 21 | import java.util.Optional; | 21 | import java.util.Optional; |
| 22 | 22 | ||
| 23 | import static com.google.common.base.Preconditions.checkNotNull; | 23 | import static com.google.common.base.Preconditions.checkNotNull; |
| 24 | +import static org.onosproject.net.intent.impl.phase.IntentProcessPhase.transferErrorCount; | ||
| 24 | 25 | ||
| 25 | /** | 26 | /** |
| 26 | * Represents a phase where intent installation has been requested. | 27 | * Represents a phase where intent installation has been requested. |
| ... | @@ -47,6 +48,8 @@ final class InstallRequest implements IntentProcessPhase { | ... | @@ -47,6 +48,8 @@ final class InstallRequest implements IntentProcessPhase { |
| 47 | 48 | ||
| 48 | @Override | 49 | @Override |
| 49 | public Optional<IntentProcessPhase> execute() { | 50 | public Optional<IntentProcessPhase> execute() { |
| 51 | + transferErrorCount(data, stored); | ||
| 52 | + | ||
| 50 | return Optional.of(new Compiling(processor, data, stored)); | 53 | return Optional.of(new Compiling(processor, data, stored)); |
| 51 | } | 54 | } |
| 52 | } | 55 | } | ... | ... |
| ... | @@ -18,6 +18,7 @@ package org.onosproject.net.intent.impl.phase; | ... | @@ -18,6 +18,7 @@ package org.onosproject.net.intent.impl.phase; |
| 18 | import org.onosproject.net.intent.IntentData; | 18 | import org.onosproject.net.intent.IntentData; |
| 19 | import org.onosproject.net.intent.impl.IntentProcessor; | 19 | import org.onosproject.net.intent.impl.IntentProcessor; |
| 20 | 20 | ||
| 21 | +import java.util.Objects; | ||
| 21 | import java.util.Optional; | 22 | import java.util.Optional; |
| 22 | 23 | ||
| 23 | /** | 24 | /** |
| ... | @@ -57,4 +58,16 @@ public interface IntentProcessPhase { | ... | @@ -57,4 +58,16 @@ public interface IntentProcessPhase { |
| 57 | } | 58 | } |
| 58 | } | 59 | } |
| 59 | 60 | ||
| 61 | + static void transferErrorCount(IntentData data, Optional<IntentData> stored) { | ||
| 62 | + if (stored.isPresent()) { | ||
| 63 | + IntentData storedData = stored.get(); | ||
| 64 | + if (Objects.equals(data.intent(), storedData.intent()) && | ||
| 65 | + Objects.equals(data.request(), storedData.request())) { | ||
| 66 | + data.setErrorCount(storedData.errorCount()); | ||
| 67 | + } else { | ||
| 68 | + data.setErrorCount(0); | ||
| 69 | + } | ||
| 70 | + } | ||
| 71 | + } | ||
| 72 | + | ||
| 60 | } | 73 | } | ... | ... |
| ... | @@ -21,6 +21,7 @@ import org.onosproject.net.intent.impl.IntentProcessor; | ... | @@ -21,6 +21,7 @@ import org.onosproject.net.intent.impl.IntentProcessor; |
| 21 | import java.util.Optional; | 21 | import java.util.Optional; |
| 22 | 22 | ||
| 23 | import static com.google.common.base.Preconditions.checkNotNull; | 23 | import static com.google.common.base.Preconditions.checkNotNull; |
| 24 | +import static org.onosproject.net.intent.impl.phase.IntentProcessPhase.transferErrorCount; | ||
| 24 | 25 | ||
| 25 | /** | 26 | /** |
| 26 | * Represents a phase of requesting a withdraw of an intent. | 27 | * Represents a phase of requesting a withdraw of an intent. |
| ... | @@ -51,6 +52,8 @@ final class WithdrawRequest implements IntentProcessPhase { | ... | @@ -51,6 +52,8 @@ final class WithdrawRequest implements IntentProcessPhase { |
| 51 | // same version i.e. they are the same | 52 | // same version i.e. they are the same |
| 52 | // Note: this call is not just the symmetric version of submit | 53 | // Note: this call is not just the symmetric version of submit |
| 53 | 54 | ||
| 55 | + transferErrorCount(data, stored); | ||
| 56 | + | ||
| 54 | if (!stored.isPresent() || stored.get().installables().isEmpty()) { | 57 | if (!stored.isPresent() || stored.get().installables().isEmpty()) { |
| 55 | switch (data.request()) { | 58 | switch (data.request()) { |
| 56 | case INSTALL_REQ: | 59 | case INSTALL_REQ: | ... | ... |
-
Please register or login to post a comment