general: add warnings when Layer1 topology is invalid

And other similar storage issues.
batfish · Feb 8, 2021 · aa02ceb · aa02ceb
1 parent 0f80e15
commit aa02ceb
Show file tree

Hide file tree

Showing 3 changed files with 48 additions and 6 deletions.
diff --git a/projects/batfish-common-protocol/src/main/java/org/batfish/common/topology/Layer1Node.java b/projects/batfish-common-protocol/src/main/java/org/batfish/common/topology/Layer1Node.java
@@ -2,7 +2,6 @@
 
 import static com.google.common.base.MoreObjects.toStringHelper;
 import static com.google.common.base.Preconditions.checkArgument;
-import static java.util.Objects.requireNonNull;
 
 import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonProperty;
@@ -22,9 +21,11 @@ public final class Layer1Node implements Comparable<Layer1Node> {
 
   @JsonCreator
   private static @Nonnull Layer1Node create(
-      @JsonProperty(PROP_HOSTNAME) String hostname,
-      @JsonProperty(PROP_INTERFACE_NAME) String interfaceName) {
-    return new Layer1Node(requireNonNull(hostname), requireNonNull(interfaceName));
+      @JsonProperty(PROP_HOSTNAME) @Nullable String hostname,
+      @JsonProperty(PROP_INTERFACE_NAME) @Nullable String interfaceName) {
+    checkArgument(hostname != null, "Missing %s", PROP_HOSTNAME);
+    checkArgument(interfaceName != null, "Missing %s", PROP_INTERFACE_NAME);
+    return new Layer1Node(hostname, interfaceName);
   }
 
   private final String _hostname;

diff --git a/projects/batfish-common-protocol/src/main/java/org/batfish/storage/FileBasedStorage.java b/projects/batfish-common-protocol/src/main/java/org/batfish/storage/FileBasedStorage.java
@@ -58,6 +58,8 @@
 import net.jpountz.lz4.LZ4FrameOutputStream;
 import org.apache.commons.io.FileUtils;
 import org.apache.commons.io.FilenameUtils;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 import org.batfish.common.BatfishException;
 import org.batfish.common.BatfishLogger;
 import org.batfish.common.BfConsts;
@@ -100,6 +102,7 @@
 /** A utility class that abstracts the underlying file system storage used by Batfish. */
 @ParametersAreNonnullByDefault
 public class FileBasedStorage implements StorageProvider {
+  private static final Logger LOGGER = LogManager.getLogger(FileBasedStorage.class);
 
   @VisibleForTesting static final Duration GC_SKEW_ALLOWANCE = Duration.ofMinutes(10L);
   private static final String ID_EXTENSION = ".id";
@@ -222,6 +225,7 @@ public SortedMap<String, Configuration> loadConfigurations(
       _logger.errorf(
           "Failed to deserialize ConvertConfigurationAnswerElement: %s",
           Throwables.getStackTraceAsString(e));
+      LOGGER.error("Failed to deserialize ConvertConfigurationAnswerElement", e);
       return null;
     }
   }
@@ -253,6 +257,11 @@ public SortedMap<String, Configuration> loadConfigurations(
       _logger.warnf(
           "Unexpected exception caught while loading interface blacklist for snapshot %s: %s",
           snapshot, Throwables.getStackTraceAsString(e));
+      LOGGER.warn(
+          String.format(
+              "Unexpected exception caught while loading interface blacklist for snapshot %s",
+              snapshot),
+          e);
       return null;
     }
   }
@@ -277,6 +286,11 @@ static boolean keyInDir(String key, String dirName) {
       _logger.warnf(
           "Unexpected exception caught while loading ISP configuration for snapshot %s: %s",
           snapshot, Throwables.getStackTraceAsString(e));
+      LOGGER.warn(
+          String.format(
+              "Unexpected exception caught while loading ISP configuration for snapshot %s",
+              snapshot),
+          e);
       return null;
     }
   }
@@ -307,6 +321,10 @@ static boolean keyInDir(String key, String dirName) {
       _logger.warnf(
           "Unexpected exception caught while loading node blacklist for snapshot %s: %s",
           snapshot, Throwables.getStackTraceAsString(e));
+      LOGGER.warn(
+          String.format(
+              "Unexpected exception caught while loading node blacklist for snapshot %s", snapshot),
+          e);
       return null;
     }
   }
@@ -336,6 +354,11 @@ static boolean keyInDir(String key, String dirName) {
       _logger.warnf(
           "Unexpected exception caught while loading layer-1 topology for snapshot %s: %s",
           snapshot, Throwables.getStackTraceAsString(e));
+      LOGGER.warn(
+          String.format(
+              "Unexpected exception caught while loading layer-1 topology for snapshot %s",
+              snapshot),
+          e);
       return null;
     } finally {
       counter.incrementAndGet();
@@ -392,6 +415,10 @@ public String loadWorkJson(NetworkId network, SnapshotId snapshot, String workId
       _logger.warnf(
           "Unexpected exception caught while loading runtime data for snapshot %s: %s",
           snapshot, Throwables.getStackTraceAsString(e));
+      LOGGER.warn(
+          String.format(
+              "Unexpected exception caught while loading runtime data for snapshot %s", snapshot),
+          e);
       return null;
     } finally {
       counter.incrementAndGet();
@@ -589,6 +616,10 @@ private boolean cachedConfigsAreCompatible(NetworkId network, SnapshotId snapsho
       _logger.warnf(
           "Unexpected exception caught while deserializing configs for snapshot %s: %s",
           snapshot, Throwables.getStackTraceAsString(e));
+      LOGGER.warn(
+          String.format(
+              "Unexpected exception caught while deserializing configs for snapshot %s", snapshot),
+          e);
       return false;
     }
   }
@@ -1838,6 +1869,8 @@ public void runGarbageCollection(Instant expungeBeforeDate) throws IOException {
                     _logger.errorf(
                         "Failed to expunge old data for network with ID '%s': %s",
                         networkId, Throwables.getStackTraceAsString(e));
+                    LOGGER.error(
+                        String.format("Failed to expunge old data for network %s", networkId), e);
                   }
                 });
       }

diff --git a/projects/batfish/src/main/java/org/batfish/topology/TopologyProviderImpl.java b/projects/batfish/src/main/java/org/batfish/topology/TopologyProviderImpl.java
@@ -12,6 +12,8 @@
 import java.util.Optional;
 import javax.annotation.Nonnull;
 import javax.annotation.ParametersAreNonnullByDefault;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 import org.batfish.common.BatfishException;
 import org.batfish.common.NetworkSnapshot;
 import org.batfish.common.plugin.IBatfish;
@@ -34,6 +36,8 @@
 
 @ParametersAreNonnullByDefault
 public final class TopologyProviderImpl implements TopologyProvider {
+  private static final Logger LOGGER = LogManager.getLogger(TopologyProviderImpl.class);
+
   /** Create a new topology provider for a given instance of {@link IBatfish} */
   public TopologyProviderImpl(IBatfish batfish, StorageProvider storage) {
     _batfish = batfish;
@@ -318,8 +322,12 @@ private Topology computeInitialLayer3Topology(NetworkSnapshot networkSnapshot) {
             .start();
     try (Scope scope = GlobalTracer.get().scopeManager().activate(span)) {
       assert scope != null; // avoid unused warning
-      return Optional.ofNullable(
-          _storage.loadLayer1Topology(networkSnapshot.getNetwork(), networkSnapshot.getSnapshot()));
+      Layer1Topology l1 =
+          _storage.loadLayer1Topology(networkSnapshot.getNetwork(), networkSnapshot.getSnapshot());
+      if (l1 == null) {
+        LOGGER.info("No Layer1 topology found for snapshot {}, or it is invalid", networkSnapshot);
+      }
+      return Optional.ofNullable(l1);
     } finally {
       span.finish();
     }