(Part 3) Command-line parsing and YAML config files in Rust by stevenengler · Pull Request #1278 · shadow/shadow

stevenengler · 2021-04-08T18:34:22Z

Replaces Shadow's existing command-line and configuration file parsing code with Rust versions using Serde and Clap (version 3.0.0-beta.2). Shadow's XML configuration file format has been replaced by YAML in the process.

This PR will be easier to review as individual commits.

Closes #773.

Changed CLI options:

Old options                                                 New options
-----------                                                 -----------

-?, --help                                                  -h, --help
--help-all
--help-sys
--help-experimental
--interface-buffer=N                    [1024000]           --interface-buffer                  ["1024000 B"]
--interface-qdisc=QDISC                 ['fifo']            --interface-qdisc                   ["fifo"]
--socket-recv-buffer=N                  [174760]            --socket-recv-buffer                ["174760 B"]
                                                            --socket-recv-autotune              [true]
--socket-send-buffer=N                  [131072]            --socket-send-buffer                ["131072 B"]
                                                            --socket-send-autotune              [true]
--max-concurrency=N                     [-1]                --max-concurrency
--set-sched-fifo                                            --use-sched-fifo                    [false]
--disable-object-counters                                   --use-object-counters               [true]
--disable-shim-syscall-handler                              --use-shim-syscall-handler          [true]
--disable-o-n-waitpid-workarounds                           --use-o-n-waitpid-workarounds       [true]
--send-explicit-block-message=[0|1]     [1]                 --use-explicit-block-message        [true]
--enable-syscall-counters                                   --use-syscall-counters              [false]
--disable-memory-manager                                    --use-memory-manager                [true]
--preload-spin-max=N                    [8096]              --preload-spin-max                  [8096]
                                                            --bootstrap-end-time                ["0 sec"]
                                                            --heartbeat-interval                ["1 sec"]
                                                            --stop-time
                                                            --bandwidth-down
                                                            --bandwidth-up
                                                            --city-code-hint
                                                            --country-code-hint
                                                            --ip-hint
                                                            --pcap-directory
                                                            --type-hint
-c, --exit-after-shm-cleanup                                --shm-cleanup
-d, --data-directory=PATH               ['shadow.data']     -d, --data-directory                ["shadow.data"]
-e, --data-template=PATH                [None]              -e, --template-directory
-g, --gdb                                                   -g, --gdb
-h, --heartbeat-frequency=N             [1]                 --heartbeat-interval                ["1 sec"]
-i, --heartbeat-log-info=LIST           ['node']            --host-heartbeat-log-info           [["node"]]
-j, --heartbeat-log-level=LEVEL         ['message']         --host-heartbeat-log-level          ["message"]
                                                            --host-log-level
-l, --log-level=LEVEL                   ['message']         -l, --log-level                     ["message"]
-r, --runahead=TIME                     [0]                 --runahead
-s, --seed=N                            [1]                 --seed                              [1]
-t, --scheduler-policy=SPOL             ['steal']           --scheduler-policy                  ["steal"]
-n, --interpose-method=METHOD           ['ptrace']          --interpose-method                  ["ptrace"]
-z, --pin-cpus                                              --use-cpu-pinning                   [false]
-w, --workers=N                         [0]                 -w, --workers                       [0]
-v, --version                                               -V, --version
                                                            --show-build-info
                                                            --show-config

Changed configuration options:

bootstraptime         bootstrap_end_time
starttime             start_time
stoptime              stop_time
arguments             args
iphint                ip_hint
citycodehint          city_code_hint
countrycodehint       country_code_hint
typehint              type_hint
loglevel              log_level
heartbeatloglevel     heartbeat_log_level
heartbeatloginfo      heartbeat_log_info
pcapdir               pcap_directory
bandwidthdown         bandwidth_down (unit changed from KiB/s to bits)
bandwidthup           bandwidth_up (unit changed from KiB/s to bits)
heartbeatfrequency    heartbeat_interval
socketrecvbuffer      socket_recv_buffer
socketsendbuffer      socket_send_buffer
interfacebuffer       interface_buffer

Changed topology options:

bandwidthup      bandwidth_up (type changed to "string", and unit changed to bits)
bandwidthdown    bandwidth_down (type changed to "string", and unit changed to bits)
packetloss       packet_loss
countrycode      country_code
citycode         city_code

Miscellaneous changes:

added a quantity field for processes
renamed the configuration file top-level options field to general, and added new experimental options under the top-level experimental field, and new host default options under the top-level host_defaults field.
hosts in the configuration file are now specified using a map/dictionary, and not a list of hosts
moved some existing options into the experimental group (Organize and remove legacy options #1220)
topology in the configuration file is no longer a list
most of the host options have moved into an options field
added a plain topology option (in addition to the existing graphml and path options), which uses a simple built-in topology (Add default single-node topology in case user does not provide one #1221)
process arguments in the configuration file can now be passed as a list of arguments
the existing XML/YAML conversion script has been modified to convert to the new YAML format, and no longer converts from YAML to XML
added a new script for converting the topology graphml file to the new format
removed the configuration file plugin list, and now each process specifies the path to the executable to run (Remove the plugins list from the root of the configuration file #1147)
disabled the XML/YAML conversion tests
the logpcap and pcapdir options have been combined into a single option pcap_directory
removed some of the short option flags (-h, -i, -j, etc)
removes legacy configuration options suck as kill and time (Remove old Shadow configuration options #1146)
values can be provided with a unit value (for example "1 Mbit", "1 megabit", etc) and if a unit isn't given, will default to the base unit (Allow variable suffixes for bandwidth config values #1161)
environment field is now per-process rather than global
arguments are no longer split by spaces

To do after this PR:

update tornettools to support new formats
add PR for new/modified conversion scripts
update and re-enable the XML/YAML conversion tests
update documentation (3.1-Shadow-Config.md, 3.2-Network-Config.md)

codecov · 2021-04-13T00:11:12Z

Codecov Report

Merging #1278 (c1df77a) into dev (fd1138d) will increase coverage by 0.44%.
The diff coverage is 70.47%.

@@            Coverage Diff             @@
##              dev    #1278      +/-   ##
==========================================
+ Coverage   55.74%   56.19%   +0.44%     
==========================================
  Files         142      140       -2     
  Lines       20960    20161     -799     
  Branches     5295     5018     -277     
==========================================
- Hits        11685    11329     -356     
+ Misses       6160     5941     -219     
+ Partials     3115     2891     -224

Flag	Coverage Δ
tests	`56.19% <70.47%> (+0.44%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/main/host/network_interface.c	`72.38% <0.00%> (ø)`
src/main/host/tracker.c	`38.85% <ø> (ø)`
src/main/core/main.c	`50.42% <40.90%> (-4.52%)`	⬇️
src/main/core/manager.c	`66.97% <59.37%> (+0.74%)`	⬆️
src/main/routing/topology.c	`44.20% <61.29%> (+2.28%)`	⬆️
src/main/core/scheduler/scheduler.c	`74.14% <66.66%> (-0.24%)`	⬇️
src/main/core/support/config_handlers.c	`83.33% <83.33%> (ø)`
src/main/core/controller.c	`79.00% <90.27%> (+18.67%)`	⬆️
src/main/core/worker.c	`77.23% <100.00%> (-0.97%)`	⬇️
src/main/host/descriptor/tcp.c	`78.06% <100.00%> (-0.34%)`	⬇️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd1138d...c1df77a. Read the comment docs.

robgjansen

This looks great! It simplifies a lot of things and will make it much simpler to configure experiments. I'm looking forward to using this new format in my experiments :)

I don't have any complex changes, but I'd like to look at any changes you make anyway :)

robgjansen · 2021-05-04T18:18:45Z

src/main/core/main.c

        return EXIT_SUCCESS;
    }

+    char* configName = clioptions_getConfig(options);


Might be worth commenting that this will never return a NULL configName.

I changed this so that it can return NULL (before it would panic!()), but also added a NULL check in C.

robgjansen · 2021-05-04T18:19:31Z

src/main/core/main.c

+        configFile = configfile_parse(configName);
+    } else {
+        configFile = configfile_parse("/dev/stdin");


Might be worth commenting why configfile_parse might return NULL.

Added a comment.

robgjansen · 2021-05-04T18:23:18Z

src/main/core/main.c

+    configfile_free(configFile);
+
+    if (clioptions_getShowConfig(options)) {
+        config_showConfig(config);


Do we want to exit after showing the config?

robgjansen · 2021-05-04T18:36:04Z

src/main/core/support/config_handlers.c

+        ConfigHandlerFn fn = g_array_index(experimentalOptions, ConfigHandlerFn, i);
+        fn(config);
+    }
+    g_array_free(experimentalOptions, true);


Do we want to set experimentalOptions to NULL to make sure we don't operate on it anymore (now that the array data has been freed)?

Good idea, added.

robgjansen · 2021-05-04T18:37:48Z

src/main/core/support/config_handlers.h

+#define OPTIONS_TOKENPASTE(x, y) x##y
+#define OPTIONS_TOKENPASTE2(x, y) OPTIONS_TOKENPASTE(x, y)


Do we want these to be used outside of this file? Or should we undef them at the end of this file?

I don't think we can undef them since they're used by the ADD_CONFIG_HANDLER macro. (The build fails when I undef them.)

robgjansen · 2021-05-04T19:12:58Z

src/test/CMakeLists.txt

         rm -rf ${SHADOW_TEST_NAME}.data \
-         && ${yaml2xml} ${SHADOW_TEST_SHADOW_CONFIG} --output - \
-         | ${CMAKE_BINARY_DIR}/src/main/shadow \
+         && ${CMAKE_BINARY_DIR}/src/main/shadow \


robgjansen · 2021-05-04T19:15:22Z

src/test/clone/clone.yaml

-  - plugin: test_clone
-    starttime: 1
-    arguments: ''
+  host:


Super-nit: can we change the name of the host to something that is more apparently a hostname instead of host?

Changed to testnode like the other tests.

src/test/determinism/determinism1.test.shadow.config.yaml

robgjansen · 2021-05-04T19:29:12Z

src/test/tor/minimal/tor-minimal.yaml

+      bandwidth_down: 819200 Kibit
+      bandwidth_up: 819200 Kibit


Some more Kibit -> Kibibit conversions here.

Never mind, Kibit is fine.

robgjansen · 2021-05-04T19:31:00Z

src/test/tor/minimal/tor-minimal.yaml

+      bandwidth_down: 819200 Kibit
+      bandwidth_up: 819200 Kibit


Should we just set this bandwidth as the default for everyone in the host_defaults part of the config? Then we wouldn't need to duplicate it for each host.

Or even better, the topology only contains a single node, and that node already has the correct bandwidth, so I think all the hosts will already get assigned that bandwidth even if we don't specify it as a host option or host_default option.

Some hosts have bandwidths of 819200 kibibit (102400 KiB/s), some have 81920 kibibit (10240 KiB/s), clients have no defined bandwidths, and the topology node has a bandwidth of 81920 kibibit. Is there a reason why the original configuration had these hosts with different bandwidths? And why some hosts had bandwiths 10x greater than the topology node? Since this is just a test, it should be fine giving them all the same bandwidth, right?

Right, I don't think the bandwidths matter much in this small test. I think they can all be the same. Not sure why they were different before - probably because I decided that many many years ago and never changed it.

Changed the topology bandwidth to 1 Gbit (rounded up from the ~800 Mbit that most hosts were using) and unset all of the other bandwidths.

robgjansen

Looks great! Thanks for the changes :)

stevenengler added Type: Enhancement New functionality or improved design Component: Main Composing the core Shadow executable labels Apr 8, 2021

stevenengler self-assigned this Apr 8, 2021

stevenengler marked this pull request as draft April 8, 2021 18:34

stevenengler force-pushed the rust-parsing branch 4 times, most recently from e58c2e1 to bfe22de Compare April 15, 2021 17:23

stevenengler changed the base branch from dev to main April 15, 2021 17:58

stevenengler changed the base branch from main to dev April 15, 2021 17:58

stevenengler force-pushed the rust-parsing branch from bfe22de to 735686b Compare April 15, 2021 18:05

stevenengler force-pushed the rust-parsing branch 5 times, most recently from aa54082 to 9e576ab Compare April 27, 2021 18:43

stevenengler changed the title ~~Command-line parsing and YAML config files in Rust~~ (Part 2) Command-line parsing and YAML config files in Rust Apr 27, 2021

stevenengler mentioned this pull request Apr 27, 2021

(Part 1) Command-line parsing and YAML config files in Rust #1309

Merged

stevenengler added this to the Updates to and new formats for Shadow configuration milestone Apr 27, 2021

stevenengler force-pushed the rust-parsing branch from 9e576ab to d310f7b Compare April 28, 2021 19:09

stevenengler changed the title ~~(Part 2) Command-line parsing and YAML config files in Rust~~ (Part 3) Command-line parsing and YAML config files in Rust May 3, 2021

stevenengler force-pushed the rust-parsing branch 3 times, most recently from 1364109 to bff5b2f Compare May 3, 2021 22:21

stevenengler requested a review from robgjansen May 3, 2021 22:28

robgjansen requested changes May 4, 2021

View reviewed changes

stevenengler requested a review from robgjansen May 5, 2021 16:49

stevenengler marked this pull request as ready for review May 5, 2021 16:50

robgjansen approved these changes May 5, 2021

View reviewed changes

stevenengler added 10 commits May 5, 2021 15:09

Missing 'SimulationTime' in the Rust config ffi

37b4aea

Removed unused Rust config option

0520af3

Switched to the Rust command-line and yaml config parsing

5502f92

Updated macros for experimental options

e8245ee

Updated topology format

f4b4458

Removed 'configuration.{c,h}' and 'options.{c,h}'

452b8e6

Updated test configuration files

da36789

Updated test yaml files

b20ed73

Disabled the xml/yaml conversion tests

3e0e973

Adjusted tor minimal test bandwidth values

c1df77a

stevenengler force-pushed the rust-parsing branch from 0bf0593 to c1df77a Compare May 5, 2021 19:09

stevenengler merged commit f6f25cd into shadow:dev May 5, 2021

stevenengler deleted the rust-parsing branch May 5, 2021 19:30

stevenengler mentioned this pull request May 6, 2021

Organize and remove legacy options #1220

Closed

		#define OPTIONS_TOKENPASTE(x, y) x##y
		#define OPTIONS_TOKENPASTE2(x, y) OPTIONS_TOKENPASTE(x, y)

Conversation

stevenengler commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

robgjansen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robgjansen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

stevenengler commented Apr 8, 2021 •

edited

Loading

codecov bot commented Apr 13, 2021 •

edited

Loading