From e8b6387f713dd87a3ee94cf7c20851b3368fc612 Mon Sep 17 00:00:00 2001 From: kheradmandG Date: Thu, 15 Feb 2024 16:47:25 -0800 Subject: [PATCH] [Dvaas] Add packet synthesizer coverage goals as part of validation parameters. 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 | 13 +++++---- dvaas/dataplane_validation.cc | 48 ++++++++++++++++++++++++------- dvaas/dataplane_validation.h | 17 +++++++++++ dvaas/packet_trace.proto | 9 ++++++ dvaas/test_run_validation.cc | 38 ++++-------------------- dvaas/test_run_validation_test.cc | 16 +++++++++++ dvaas/test_vector.proto | 2 ++ dvaas/traffic_generator.cc | 12 ++++++++ dvaas/traffic_generator.h | 4 ++- dvaas/validation_result.cc | 9 ++++-- dvaas/validation_result.h | 14 +++++---- 11 files changed, 124 insertions(+), 58 deletions(-) create mode 100644 dvaas/packet_trace.proto diff --git a/dvaas/BUILD.bazel b/dvaas/BUILD.bazel index 2edd1d8b0..e05a43f18 100644 --- a/dvaas/BUILD.bazel +++ b/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", ], diff --git a/dvaas/dataplane_validation.cc b/dvaas/dataplane_validation.cc index 6592abdad..0670995e1 100644 --- a/dvaas/dataplane_validation.cc +++ b/dvaas/dataplane_validation.cc @@ -17,7 +17,6 @@ #include #include #include -#include #include #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 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 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; } diff --git a/dvaas/dataplane_validation.h b/dvaas/dataplane_validation.h index 600e74a0e..e7ca56b2d 100644 --- a/dvaas/dataplane_validation.h +++ b/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 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 + coverage_goals_override; }; // Forward declaration. See below for description. @@ -225,6 +233,8 @@ class DataplaneValidationBackend { const p4::v1::ForwardingPipelineConfig& p4_symbolic_config, absl::Span ports, const OutputWriterFunctionType& write_stats, + const std::optional& + coverage_goals_override, std::optional 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 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; }; diff --git a/dvaas/packet_trace.proto b/dvaas/packet_trace.proto new file mode 100644 index 000000000..5242f4e8c --- /dev/null +++ b/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; +} diff --git a/dvaas/test_run_validation.cc b/dvaas/test_run_validation.cc index a56089f0e..51381bdd5 100644 --- a/dvaas/test_run_validation.cc +++ b/dvaas/test_run_validation.cc @@ -15,9 +15,7 @@ #include "dvaas/test_run_validation.h" #include -#include #include -#include #include #include @@ -31,9 +29,7 @@ #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" @@ -41,10 +37,10 @@ #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 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; diff --git a/dvaas/test_run_validation_test.cc b/dvaas/test_run_validation_test.cc index 9b0c9d668..65d4c807e 100644 --- a/dvaas/test_run_validation_test.cc +++ b/dvaas/test_run_validation_test.cc @@ -132,6 +132,22 @@ TEST(TestRunValidationTest, HasSubstr("mismatched ports:")); } +TEST(TestRunValidationTest, DifferentPortOrderOfPacketsIsOk) { + EXPECT_FALSE(ValidateTestRun(gutil::ParseProtoOrDie(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(R"pb( diff --git a/dvaas/test_vector.proto b/dvaas/test_vector.proto index 2fcc0774e..a35c8af2c 100644 --- a/dvaas/test_vector.proto +++ b/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. diff --git a/dvaas/traffic_generator.cc b/dvaas/traffic_generator.cc index 137ef230d..a23d3f88f 100644 --- a/dvaas/traffic_generator.cc +++ b/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 diff --git a/dvaas/traffic_generator.h b/dvaas/traffic_generator.h index 0555b5753..3b01112ab 100644 --- a/dvaas/traffic_generator.h +++ b/dvaas/traffic_generator.h @@ -152,7 +152,9 @@ class SimpleTrafficGenerator : public TrafficGenerator { absl::StatusOr GetValidationResult() override; absl::StatusOr GetAndClearValidationResult() override; -private: + ~SimpleTrafficGenerator(); + + private: std::unique_ptr backend_; std::unique_ptr testbed_configurator_; // Test vectors created as a result of (latest) call to `Init`. Calls to diff --git a/dvaas/validation_result.cc b/dvaas/validation_result.cc index 63805b5e1..38fcd37c2 100644 --- a/dvaas/validation_result.cc +++ b/dvaas/validation_result.cc @@ -18,17 +18,22 @@ #include #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) { diff --git a/dvaas/validation_result.h b/dvaas/validation_result.h index d47cc62df..9afcff743 100644 --- a/dvaas/validation_result.h +++ b/dvaas/validation_result.h @@ -21,13 +21,10 @@ #include #include -#include "absl/container/flat_hash_set.h" #include "absl/status/status.h" -#include "absl/types/span.h" #include "dvaas/test_run_validation.h" #include "dvaas/test_vector.pb.h" #include "dvaas/test_vector_stats.h" -#include "google/protobuf/descriptor.h" #include "p4_symbolic/packet_synthesizer/packet_synthesizer.pb.h" namespace dvaas { @@ -69,9 +66,14 @@ class [[nodiscard]] ValidationResult { // as it includes additional information to ease debugging. std::vector GetAllFailures() const; - // Constructs a `ValidationResult` from the given `test_vectors`. Ignores - // `ignored_fields` and `ignored_metadata` during validation, see - // `test_run_validation.h` for details. + // Constructs a `ValidationResult` from the given `test_outcomes` and + // `packet_synthesis_result`. + ValidationResult(const PacketTestOutcomes& test_outcomes, + const PacketSynthesisResult& packet_synthesis_result); + + // Constructs a `ValidationResult` from the given `test_runs` and + // `packet_synthesis_result`. Uses `diff_params` while validating the test + // runs. See `test_run_validation.h` for details. ValidationResult(const PacketTestRuns& test_runs, const SwitchOutputDiffParams& diff_params, const PacketSynthesisResult& packet_synthesis_result);