Skip to content

Commit

Permalink
Beef up config validator
Browse files Browse the repository at this point in the history
It was performing relatively unsophisticated checks. Make it so that it
can catch more subtle issues with timestamp channel configurations.

Change-Id: Ie42fba108c6bcb0d1cad2484dc435bac10a003ba
Signed-off-by: James Kuszmaul <[email protected]>
  • Loading branch information
jkuszmaul committed May 18, 2023
1 parent ab23618 commit 827bd21
Show file tree
Hide file tree
Showing 18 changed files with 1,720 additions and 20 deletions.
5 changes: 5 additions & 0 deletions aos/network/message_bridge_test_common.json
Original file line number Diff line number Diff line change
Expand Up @@ -75,28 +75,33 @@
{
"name": "/pi1/aos/remote_timestamps/pi2/pi1/aos/aos-message_bridge-Timestamp",
"type": "aos.message_bridge.RemoteMessage",
"logger": "NOT_LOGGED",
"source_node": "pi1",
"frequency": 15
},
{
"name": "/pi2/aos/remote_timestamps/pi1/pi2/aos/aos-message_bridge-Timestamp",
"type": "aos.message_bridge.RemoteMessage",
"logger": "NOT_LOGGED",
"source_node": "pi2",
"frequency": 15
},
{
"name": "/pi1/aos/remote_timestamps/pi2/test/aos-examples-Ping",
"type": "aos.message_bridge.RemoteMessage",
"logger": "NOT_LOGGED",
"source_node": "pi1"
},
{
"name": "/pi2/aos/remote_timestamps/pi1/test/aos-examples-Pong",
"type": "aos.message_bridge.RemoteMessage",
"logger": "NOT_LOGGED",
"source_node": "pi2"
},
{
"name": "/pi1/aos/remote_timestamps/pi2/unreliable/aos-examples-Ping",
"type": "aos.message_bridge.RemoteMessage",
"logger": "NOT_LOGGED",
"source_node": "pi1"
},
{
Expand Down
51 changes: 47 additions & 4 deletions aos/util/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -505,11 +505,8 @@ cc_binary(
srcs = ["config_validator.cc"],
target_compatible_with = ["@platforms//os:linux"],
deps = [
"//aos:init",
":config_validator_lib",
"//aos:json_to_flatbuffer",
"//aos/events:simulated_event_loop",
"//aos/events/logging:log_reader",
"//aos/events/logging:log_writer",
"//aos/testing:googletest",
"@com_github_gflags_gflags//:gflags",
"@com_github_google_glog//:glog",
Expand Down Expand Up @@ -538,3 +535,49 @@ cc_library(
"//aos/events/logging:log_writer",
],
)

flatbuffer_cc_library(
name = "config_validator_config_fbs",
srcs = ["config_validator_config.fbs"],
target_compatible_with = ["@platforms//os:linux"],
visibility = ["//visibility:public"],
)

cc_library(
name = "config_validator_lib",
testonly = True,
srcs = ["config_validator_lib.cc"],
hdrs = ["config_validator_lib.h"],
deps = [
":config_validator_config_fbs",
":simulation_logger",
"//aos/events:simulated_event_loop",
"//aos/events/logging:log_reader",
"//aos/events/logging:log_writer",
"//aos/network:timestamp_channel",
"//aos/testing:tmpdir",
"@com_github_google_glog//:glog",
"@com_google_googletest//:gtest",
],
)

cc_test(
name = "config_validator_lib_test",
srcs = ["config_validator_lib_test.cc"],
data = [
"//aos/util/test_data:multinode_common_logger",
"//aos/util/test_data:multinode_extraneous_timestamp",
"//aos/util/test_data:multinode_invalid_timestamp_logger_list",
"//aos/util/test_data:multinode_no_logged_timestamps",
"//aos/util/test_data:multinode_no_statistics",
"//aos/util/test_data:multinode_timestamp_typo",
"//aos/util/test_data:valid_multinode_config",
"//aos/util/test_data:valid_singlenode_config",
],
deps = [
":config_validator_lib",
"//aos:json_to_flatbuffer",
"//aos/testing:googletest",
"//aos/testing:path",
],
)
21 changes: 8 additions & 13 deletions aos/util/config_validator.cc
Original file line number Diff line number Diff line change
@@ -1,16 +1,9 @@
#include <chrono>

#include "aos/configuration.h"
#include "aos/events/logging/log_reader.h"
#include "aos/events/logging/log_writer.h"
#include "aos/events/simulated_event_loop.h"
#include "aos/init.h"
#include "aos/json_to_flatbuffer.h"
#include "aos/network/team_number.h"
#include "gflags/gflags.h"
#include "gtest/gtest.h"
#include "aos/util/config_validator_lib.h"

DEFINE_string(config, "", "Name of the config file to replay using.");
DEFINE_string(validation_config, "{}",
"JSON config to use to validate the config.");
/* This binary is used to validate that all of the
needed remote timestamps channels are in the config
to log the timestamps.
Expand All @@ -26,9 +19,11 @@ TEST(ConfigValidatorTest, ReadConfig) {
const aos::FlatbufferDetachedBuffer<aos::Configuration> config =
aos::configuration::ReadConfig(FLAGS_config);

aos::SimulatedEventLoopFactory factory(&config.message());

factory.RunFor(std::chrono::seconds(1));
const aos::FlatbufferDetachedBuffer<aos::util::ConfigValidatorConfig>
validator_config =
aos::JsonToFlatbuffer<aos::util::ConfigValidatorConfig>(
FLAGS_validation_config);
aos::util::ConfigIsValid(&config.message(), &validator_config.message());
}

// TODO(milind): add more tests, the above one doesn't
Expand Down
55 changes: 55 additions & 0 deletions aos/util/config_validator_config.fbs
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
namespace aos.util;

// This file defines a schema for what to validate when we run the
// config_validator against an AOS config.
// The primary purpose of this config is to allow the user to specify what
// sets of nodes they expect to be able to log on so that we can validate the
// logging configurations. In the future this may also include flags to indicate
// how aggressively to do certain checks.
//
// This flatbuffer should not exist in serialized form anywhere, and so is
// safe to modify in non-backwards-compatible ways.

// Species a set of nodes that you should be able to combine the logs from and
// subsequently replay. E.g., this allows you to write a check that says
// "If you combine logs from pi2 & pi4, you should be able to replay data from
// nodes pi2, pi4, and pi6"; or
// "When logs from all nodes are combined, you should be able to replay data
// for all nodes;" or
// "Each node should log all the data needed to replay its own data"
// (this would require muliple LoggerNodeSetValidation's).
//
// Each LoggerNodeSetValidation table represents a single set of logging nodes
// that should be able to replay data on some number of other nodes. An empty
// list of loggers or replay_nodes indicates "all nodes." The above examples
// could then be represented by, e.g.:
// "pi2 & pi4 -> pi2, pi4, & pi6":
// {"loggers": ["pi2", "pi4"], "replay_nodes": ["pi2", "pi4", "pi6"]}
// "all -> all": {"logger": [], "replay_nodes": []}
// "each node -> itself": [
// {"logger": ["pi1"], "replay_nodes": ["pi1"]},
// {"logger": ["pi2"], "replay_nodes": ["pi2"]},
// {"logger": ["pi3"], "replay_nodes": ["pi3"]},
// {"logger": ["pi4"], "replay_nodes": ["pi4"]}]
table LoggerNodeSetValidation {
loggers:[string] (id: 0);
replay_nodes:[string] (id: 1);
}

// This table specifies which
table LoggingConfigValidation {
// If true, all channels should be logged by some valid set of loggers.
// Essentially, this is checking that no channels are configured to be
// NOT_LOGGED except for remote timestamp channels.
all_channels_logged:bool = true (id: 0);
// A list of all the sets of logger nodes that we care about. Typically this
// should at least include an entry that says that "logs from all nodes should
// combine to allow you to replay all nodes."
logger_sets:[LoggerNodeSetValidation] (id: 1);
}

table ConfigValidatorConfig {
logging:LoggingConfigValidation (id: 0);
}

root_type ConfigValidatorConfig;
Loading

0 comments on commit 827bd21

Please sign in to comment.