[Dvaas] Add packet synthesizer coverage goals as part of validation p…

…arameters. Make SimpleTrafficGenerator tolerate object destruction while traffic is flowing. Add a `ValidationResult` constructor that takes `test_outcomes` as an argument. Disable bmv2 log by default. Define a `StorePacketTrace` function that store P4 simulation packet traces for given input (bmv2 logs for now). Make dvaas::ValidateDataplane call it for the first failure. Fix packet comparison. Add PacketTrace proto. Store bmv2 log dir in bmv2 wrapper.

dvaas/BUILD.bazel

@@ -56,6 +56,7 @@ cc_library(
         "//p4_pdpi:p4_runtime_session",
         "//p4_pdpi:p4_runtime_session_extras",
         "//p4_pdpi/packetlib:packetlib_cc_proto",
+        "//p4_symbolic/packet_synthesizer:coverage_goal_cc_proto",
         "//p4_symbolic/packet_synthesizer:packet_synthesizer_cc_proto",
         "//tests/lib:switch_test_setup_helpers",
         "//thinkit:mirror_testbed",
@@ -85,10 +86,7 @@ cc_library(
         "//p4_symbolic/packet_synthesizer:packet_synthesizer_cc_proto",
         "@com_github_google_glog//:glog",
         "@com_google_absl//absl/algorithm:container",
-        "@com_google_absl//absl/container:flat_hash_set",
         "@com_google_absl//absl/status",
-        "@com_google_absl//absl/types:span",
-        "@com_google_protobuf//:protobuf",
     ],
 )
 
@@ -131,9 +129,9 @@ cc_library(
     hdrs = ["test_run_validation.h"],
     deps = [
         ":output_writer",
-        ":test_vector",
         ":test_vector_cc_proto",
         "//gutil:proto",
+        "//gutil:proto_ordering",
         "//gutil:status",
         "//p4_pdpi:ir_cc_proto",
         "//p4_pdpi/packetlib:packetlib_cc_proto",
@@ -146,7 +144,6 @@ cc_library(
         "@com_google_absl//absl/types:span",
         "@com_google_googletest//:gtest",
         "@com_google_protobuf//:protobuf",
-        "@com_googlesource_code_re2//:re2",
     ],
 )
 
@@ -177,10 +174,16 @@ cc_library(
     ],
 )
 
+proto_library(
+    name = "packet_trace_proto",
+    srcs = ["packet_trace.proto"],
+)
+
 proto_library(
     name = "test_vector_proto",
     srcs = ["test_vector.proto"],
     deps = [
+        ":packet_trace_proto",
         "//p4_pdpi:ir_proto",
         "//p4_pdpi/packetlib:packetlib_proto",
     ],

dvaas/dataplane_validation.cc

@@ -17,7 +17,6 @@
 #include <memory>
 #include <optional>
 #include <string>
-#include <utility>
 #include <vector>
 
 #include "absl/status/status.h"
@@ -29,6 +28,7 @@
 #include "dvaas/packet_injection.h"
 #include "dvaas/port_id_map.h"
 #include "dvaas/switch_api.h"
+#include "dvaas/test_run_validation.h"
 #include "dvaas/test_vector.h"
 #include "dvaas/test_vector.pb.h"
 #include "dvaas/user_provided_packet_test_vector.h"
@@ -195,14 +195,14 @@ absl::StatusOr<GenerateTestVectorsResult> GenerateTestVectors(
 
   // Synthesize test packets.
   LOG(INFO) << "Synthesizing test packets";
-  ASSIGN_OR_RETURN(generate_test_vectors_result.packet_synthesis_result,
-                   backend.SynthesizePackets(
-                       ir_p4info, entities, p4_spec.p4_symbolic_config, ports,
-                       [&](absl::string_view stats) {
-                         return writer.AppendToTestArtifact(
-                             "test_packet_stats.txt", stats);
-                       },
-                       params.packet_synthesis_time_limit));
+  ASSIGN_OR_RETURN(
+      generate_test_vectors_result.packet_synthesis_result,
+      backend.SynthesizePackets(
+          ir_p4info, entities, p4_spec.p4_symbolic_config, ports,
+          [&](absl::string_view stats) {
+            return writer.AppendToTestArtifact("test_packet_stats.txt", stats);
+          },
+          params.coverage_goals_override, params.packet_synthesis_time_limit));
 
   RETURN_IF_ERROR(writer.AppendToTestArtifact(
       "synthesized_packets.txt",
@@ -293,12 +293,38 @@ absl::StatusOr<ValidationResult> DataplaneValidator::ValidateDataplane(
   RETURN_IF_ERROR(writer->AppendToTestArtifact("test_runs.textproto",
                                                test_runs.DebugString()));
 
+  // Validate test runs to create test outcomes.
+  dvaas::PacketTestOutcomes test_outcomes;
+  test_outcomes.mutable_outcomes()->Reserve(test_runs.test_runs_size());
+  for (const auto& test_run : test_runs.test_runs()) {
+    dvaas::PacketTestOutcome& test_outcome = *test_outcomes.add_outcomes();
+    *test_outcome.mutable_test_run() = test_run;
+    *test_outcome.mutable_test_result() =
+        ValidateTestRun(test_run, params.switch_output_diff_params);
+  }
+
   ValidationResult validation_result(
-      std::move(test_runs), params.switch_output_diff_params,
-      generate_test_vectors_result.packet_synthesis_result);
+      test_outcomes, generate_test_vectors_result.packet_synthesis_result);
   RETURN_IF_ERROR(writer->AppendToTestArtifact(
       "test_vector_failures.txt",
       absl::StrJoin(validation_result.GetAllFailures(), "\n\n")));
+
+  // Store the packet trace for the first failed test packet (if any).
+  ASSIGN_OR_RETURN(P4Specification p4_spec,
+                   InferP4Specification(params, *backend_, sut));
+  ASSIGN_OR_RETURN(pdpi::IrP4Info ir_p4info, pdpi::GetIrP4Info(*sut.p4rt));
+  for (const dvaas::PacketTestOutcome& test_outcome :
+       test_outcomes.outcomes()) {
+    if (test_outcome.test_result().has_failure()) {
+      LOG(INFO) << "Storing packet trace for the first failed test packet";
+      const SwitchInput& switch_input =
+          test_outcome.test_run().test_vector().input();
+      RETURN_IF_ERROR(backend_->StorePacketTrace(p4_spec.bmv2_config, ir_p4info,
+                                                 entities, switch_input));
+      break;
+    }
+  }
+
   return validation_result;
 }
 

dvaas/dataplane_validation.h

@@ -28,6 +28,7 @@ limitations under the License.
 
 #include "absl/container/flat_hash_map.h"
 #include "absl/container/flat_hash_set.h"
+#include "absl/status/status.h"
 #include "absl/status/statusor.h"
 #include "absl/time/time.h"
 #include "absl/types/span.h"
@@ -44,6 +45,7 @@ limitations under the License.
 #include "lib/p4rt/p4rt_port.h"
 #include "p4_pdpi/ir.pb.h"
 #include "p4_pdpi/packetlib/packetlib.pb.h"
+#include "p4_symbolic/packet_synthesizer/coverage_goal.pb.h"
 #include "p4_symbolic/packet_synthesizer/packet_synthesizer.pb.h"
 #include "thinkit/mirror_testbed.h"
 
@@ -109,6 +111,12 @@ struct DataplaneValidationParams {
   // Otherwise, if packet synthesis timed out, the synthesis results cover the
   // coverage goals only partially.
   std::optional<absl::Duration> packet_synthesis_time_limit = std::nullopt;
+
+  // Coverage goals override for automated packet synthesis (go/coverage-goals).
+  // If nullopt the backend uses its default goals, which should work well
+  // enough in most cases.
+  std::optional<p4_symbolic::packet_synthesizer::CoverageGoals>
+      coverage_goals_override;
 };
 
 // Forward declaration. See below for description.
@@ -225,6 +233,8 @@ class DataplaneValidationBackend {
       const p4::v1::ForwardingPipelineConfig& p4_symbolic_config,
       absl::Span<const pins_test::P4rtPortId> ports,
       const OutputWriterFunctionType& write_stats,
+      const std::optional<p4_symbolic::packet_synthesizer::CoverageGoals>&
+          coverage_goals_override,
       std::optional<absl::Duration> time_limit = std::nullopt) const = 0;
 
   // Generates a map of test ID to PacketTestVector with output prediction
@@ -270,6 +280,13 @@ class DataplaneValidationBackend {
   virtual absl::StatusOr<P4Specification>
   InferP4Specification(SwitchApi &sut) const = 0;
 
+  // Stores the P4 simulation packet trace for the given config, entries, and
+  // input packet.
+  virtual absl::Status StorePacketTrace(
+      const p4::v1::ForwardingPipelineConfig& bmv2_compatible_config,
+      const pdpi::IrP4Info& ir_p4info, const pdpi::IrEntities& ir_entities,
+      const SwitchInput& switch_input) const = 0;
+
   virtual ~DataplaneValidationBackend() = default;
 };
 

dvaas/packet_trace.proto

@@ -0,0 +1,9 @@
+syntax = "proto3";
+
+package dvaas;
+
+// A proto representation of the trace of a packet through a P4 packet
+// processor.
+message PacketTrace {
+  string bmv2_textual_log = 1;
+}

dvaas/test_run_validation.cc

@@ -15,9 +15,7 @@
 #include "dvaas/test_run_validation.h"
 
 #include <optional>
-#include <ostream>
 #include <string>
-#include <tuple>
 #include <utility>
 #include <vector>
 
@@ -31,20 +29,18 @@
 #include "absl/strings/strip.h"
 #include "absl/strings/substitute.h"
 #include "absl/types/optional.h"
-#include "absl/types/span.h"
 #include "dvaas/output_writer.h"
-#include "dvaas/test_vector.h"
 #include "dvaas/test_vector.pb.h"
 #include "glog/logging.h"
 #include "gmock/gmock.h"
 #include "google/protobuf/descriptor.h"
 #include "google/protobuf/util/message_differencer.h"
 #include "gtest/gtest.h"
 #include "gutil/proto.h"
+#include "gutil/proto_ordering.h"
 #include "gutil/status.h"
 #include "p4_pdpi/ir.pb.h"
 #include "p4_pdpi/packetlib/packetlib.pb.h"
-#include "re2/re2.h"
 
 namespace dvaas {
 
@@ -56,14 +52,6 @@ using ::testing::MatchResultListener;
 
 // -- Detailed comparison of actual vs expected `SwitchOutput`s ----------------
 
-bool PacketLessThan(const Packet* a, const Packet* b) {
-  return a->hex() < b->hex();
-}
-
-bool PacketInLessThan(const PacketIn* a, const PacketIn* b) {
-  return a->hex() < b->hex();
-}
-
 // Returns a copy of the given `string` with all newlines indented by
 // (an additional) `indentation` number of spaces. Empty lines are not indented.
 std::string Indent(int indentation, absl::string_view string) {
@@ -108,16 +96,10 @@ bool CompareSwitchOutputs(SwitchOutput actual_output,
     }
   }
 
-  std::sort(actual_output.mutable_packets()->pointer_begin(),
-            actual_output.mutable_packets()->pointer_end(), PacketLessThan);
-  std::sort(expected_output.mutable_packets()->pointer_begin(),
-            expected_output.mutable_packets()->pointer_end(), PacketLessThan);
-  std::sort(actual_output.mutable_packet_ins()->pointer_begin(),
-            actual_output.mutable_packet_ins()->pointer_end(),
-            PacketInLessThan);
-  std::sort(expected_output.mutable_packet_ins()->pointer_begin(),
-            expected_output.mutable_packet_ins()->pointer_end(),
-            PacketInLessThan);
+  gutil::InefficientProtoSort(*actual_output.mutable_packets());
+  gutil::InefficientProtoSort(*expected_output.mutable_packets());
+  gutil::InefficientProtoSort(*actual_output.mutable_packet_ins());
+  gutil::InefficientProtoSort(*expected_output.mutable_packet_ins());
 
   for (int i = 0; i < expected_output.packets_size(); ++i) {
     const Packet& actual_packet = actual_output.packets(i);
@@ -333,16 +315,6 @@ static constexpr absl::string_view kExpectationBanner =
 
 }  // namespace
 
-absl::StatusOr<int> ExtractTestPacketTag(const packetlib::Packet& packet) {
-  // Regexp to extract the ID from a test packet's payload.
-  constexpr LazyRE2 kTestPacketIdRegexp{R"(test packet #([0-9]+):)"};
-  int tag;
-  if (RE2::PartialMatch(packet.payload(), *kTestPacketIdRegexp, &tag))
-    return tag;
-  return absl::InvalidArgumentError(absl::StrCat(
-      "Payload does not contain a packet id: ", packet.DebugString()));
-}
-
 PacketTestValidationResult ValidateTestRun(
     const PacketTestRun& test_run, const SwitchOutputDiffParams& diff_params) {
   PacketTestValidationResult result;

dvaas/test_run_validation_test.cc

@@ -132,6 +132,22 @@ TEST(TestRunValidationTest,
               HasSubstr("mismatched ports:"));
 }
 
+TEST(TestRunValidationTest, DifferentPortOrderOfPacketsIsOk) {
+  EXPECT_FALSE(ValidateTestRun(gutil::ParseProtoOrDie<PacketTestRun>(R"pb(
+                 test_vector {
+                   acceptable_outputs {
+                     packets { port: "1" }
+                     packets { port: "2" }
+                   }
+                 }
+                 actual_output {
+                   packets { port: "2" }
+                   packets { port: "1" }
+                 }
+               )pb"))
+                   .has_failure());
+}
+
 TEST(TestRunValidationTest,
      MissingPacketInsAreIgnoredIfAndOnlyIfIgnorePacketInsIsSet) {
   const PacketTestRun test_run = gutil::ParseProtoOrDie<PacketTestRun>(R"pb(

dvaas/test_vector.proto

@@ -16,6 +16,7 @@ syntax = "proto3";
 
 package dvaas;
 
+import "dvaas/packet_trace.proto";
 import "p4_pdpi/ir.proto";
 import "p4_pdpi/packetlib/packetlib.proto";
 
@@ -113,6 +114,7 @@ message PacketTestValidationResult {
 message PacketTestOutcome {
   PacketTestRun test_run = 1;
   PacketTestValidationResult test_result = 2;
+  PacketTrace p4_packet_trace = 3;
 }
 
 // A list of test outcomes.

dvaas/traffic_generator.cc

@@ -197,4 +197,16 @@ SimpleTrafficGenerator::GetAndClearValidationResult() {
       generate_test_vectors_result_.packet_synthesis_result);
 }
 
+SimpleTrafficGenerator::~SimpleTrafficGenerator() {
+  if (GetState() == kTrafficFlowing) {
+    LOG(WARNING)
+        << "SimpleTrafficGenerator destructed while traffic is flowing. "
+           "Stopping traffic.";
+    absl::Status status = StopTraffic();
+    if (!status.ok()) {
+      LOG(FATAL) << "Failed to stop traffic: " << status;  // Crash OK.
+    }
+  }
+}
+
 }  // namespace dvaas

dvaas/traffic_generator.h

@@ -152,7 +152,9 @@ class SimpleTrafficGenerator : public TrafficGenerator {
   absl::StatusOr<ValidationResult> GetValidationResult() override;
   absl::StatusOr<ValidationResult> GetAndClearValidationResult() override;
 
-private:
+  ~SimpleTrafficGenerator();
+
+ private:
   std::unique_ptr<DataplaneValidationBackend> backend_;
   std::unique_ptr<MirrorTestbedConfigurator> testbed_configurator_;
   // Test vectors created as a result of (latest) call to `Init`. Calls to

dvaas/validation_result.cc

@@ -18,17 +18,22 @@
 #include <vector>
 
 #include "absl/algorithm/container.h"
-#include "absl/container/flat_hash_set.h"
 #include "absl/status/status.h"
 #include "dvaas/test_run_validation.h"
 #include "dvaas/test_vector.pb.h"
 #include "dvaas/test_vector_stats.h"
 #include "glog/logging.h"
-#include "google/protobuf/descriptor.h"
 #include "gutil/status.h"
 
 namespace dvaas {
 
+ValidationResult::ValidationResult(
+    const PacketTestOutcomes& test_outcomes,
+    const PacketSynthesisResult& packet_synthesis_result)
+    : test_outcomes_(test_outcomes),
+      test_vector_stats_(ComputeTestVectorStats(test_outcomes)),
+      packet_synthesis_result_(packet_synthesis_result) {}
+
 ValidationResult::ValidationResult(
     const PacketTestRuns& test_runs, const SwitchOutputDiffParams& diff_params,
     const PacketSynthesisResult& packet_synthesis_result) {