Tool that converts XML shadow config file to YAML. by florian-vuillemot · Pull Request #803 · shadow/shadow

florian-vuillemot · 2020-05-20T12:32:01Z

#773
Example files included are here to simplify discussion on the script result

florian-vuillemot · 2020-05-20T12:35:00Z

@robgjansen @sporksmith should I keep the compatibility with Python2 ?

robgjansen · 2020-05-21T16:00:33Z

No, I think we've already decided to no longer support python2, so as long as it supports python3 I think that's OK.

robgjansen · 2020-05-21T17:21:17Z

Does the python2 linter test add value, even though we don't require it to pass? If not, perhaps it's time to remove it. What do you think @sporksmith ?

sporksmith · 2020-05-21T17:59:14Z

Does the python2 linter test add value, even though we don't require it to pass? If not, perhaps it's time to remove it. What do you think @sporksmith ?

Dropping it SGTM :)

florian-vuillemot · 2020-05-21T21:03:12Z

Great ! About the code, the output is not exactly like in your example @robgjansen , the format is like the yaml package provided. Would you want to improve it ? And if yes, how ?

robgjansen

We should also get @sporksmith's comments here, since I think he has more experience with YAML than me and probably also a stronger opinion about how we ought to format things to improve usability.

robgjansen · 2020-05-26T00:16:59Z

src/test/tools/convert_xml_to_yaml/file.yaml

+  process:
+  - arguments: tgen.server.graphml.xml
+    plugin: tgen
+    starttime: '1'
+- id: client
+  process:
+  - arguments: tgen.client.graphml.xml
+    plugin: tgen
+    starttime: '2'
+option:
+  stoptime: '3600'


This is sort of backwards from what I would expect. The numbers are given in quotes, and the strings are not. Is that a standard YAML thing?

Bare (unquoted) strings are allowed in YAML, but I think can't begin with digits. It looks like the conversion is forcing everything to be a string, and leaving off the quotes where they aren't needed.

I agree it'd be better for it to make the integer-valued fields integers instead of forcing them to be strings.

I fixed it. I'm not satisfied with the conversion function. Do you have a better way ?

robgjansen · 2020-05-26T00:17:57Z

src/test/tools/convert_xml_to_yaml/file.yaml

+- graphml: "<?xml version=\"1.0\" encoding=\"utf-8\"?><graphml xmlns=\"http://graphml.graphdrawing.org/xmlns\"\
+    \ xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\" xsi:schemaLocation=\"\
+    http://graphml.graphdrawing.org/xmlns http://graphml.graphdrawing.org/xmlns/1.0/graphml.xsd\"\
+    >\n  <key attr.name=\"packetloss\" attr.type=\"double\" for=\"edge\" id=\"d6\"\
+    \ />\n  <key attr.name=\"latency\" attr.type=\"double\" for=\"edge\" id=\"d5\"\
+    \ />\n  <key attr.name=\"packetloss\" attr.type=\"double\" for=\"node\" id=\"\
+    d4\" />\n  <key attr.name=\"countrycode\" attr.type=\"string\" for=\"node\" id=\"\
+    d3\" />\n  <key attr.name=\"bandwidthdown\" attr.type=\"int\" for=\"node\" id=\"\
+    d2\" />\n  <key attr.name=\"bandwidthup\" attr.type=\"int\" for=\"node\" id=\"\
+    d1\" />\n  <key attr.name=\"ip\" attr.type=\"string\" for=\"node\" id=\"d0\" />\n\
+    \  <graph edgedefault=\"undirected\">\n    <node id=\"isp\">\n      <data key=\"\
+    d0\">0.0.0.0</data>\n      <data key=\"d1\">2251</data>\n      <data key=\"d2\"\
+    >17038</data>\n      <data key=\"d3\">US</data>\n      <data key=\"d4\">0.0</data>\n\
+    \    </node>\n    <edge source=\"isp\" target=\"isp\">\n      <data key=\"d5\"\
+    >50.0</data>\n      <data key=\"d6\">0.01</data>\n    </edge>\n  </graph>\n</graphml>\n"


This is MUCH harder to read than it was before. It's basically unreadable. Is there a way to automatically format this so it is pretty-printed?

+1. In addition to keeping or improving the internal formatting of the XML from the source, I think we can get rid of the \ns and escaped quotes by using a multiline string. https://yaml-multiline.info/

Good advice, it's much better now ! Thank you

robgjansen · 2020-05-26T00:20:17Z

src/tools/convert_xml_to_yaml.py

+    if len(sys.argv) != 3:
+        print(f'Usage: {sys.argv[0]} <xml_filename> <yaml_filename>', file=sys.stderr)
+        exit(1)
+
+    xml_filename = sys.argv[1]
+    yaml_filename = sys.argv[2]


Would it be useful to use the argparse package here to make it easier for people to use to convert their older files? Or is that a waste of time? (I find it useful whenever tools have help menus, which you get almost for free with argparse.)

sporksmith

I think we'd discussed creating a YAML->XML conversion tool first, so that the config can be written and maintained in YAML and then converted to shadow's current format "just in time"?

This tool could also be useful for rewriting old configs, but I think that might be getting ahead of ourselves a bit.

sporksmith · 2020-05-26T16:43:03Z

src/test/tools/convert_xml_to_yaml/file.yaml

+  process:
+  - arguments: tgen.server.graphml.xml
+    plugin: tgen
+    starttime: '1'
+- id: client
+  process:
+  - arguments: tgen.client.graphml.xml
+    plugin: tgen
+    starttime: '2'
+option:
+  stoptime: '3600'


Bare (unquoted) strings are allowed in YAML, but I think can't begin with digits. It looks like the conversion is forcing everything to be a string, and leaving off the quotes where they aren't needed.

I agree it'd be better for it to make the integer-valued fields integers instead of forcing them to be strings.

sporksmith · 2020-05-26T16:43:13Z

src/test/tools/convert_xml_to_yaml/file.yaml

+- graphml: "<?xml version=\"1.0\" encoding=\"utf-8\"?><graphml xmlns=\"http://graphml.graphdrawing.org/xmlns\"\
+    \ xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\" xsi:schemaLocation=\"\
+    http://graphml.graphdrawing.org/xmlns http://graphml.graphdrawing.org/xmlns/1.0/graphml.xsd\"\
+    >\n  <key attr.name=\"packetloss\" attr.type=\"double\" for=\"edge\" id=\"d6\"\
+    \ />\n  <key attr.name=\"latency\" attr.type=\"double\" for=\"edge\" id=\"d5\"\
+    \ />\n  <key attr.name=\"packetloss\" attr.type=\"double\" for=\"node\" id=\"\
+    d4\" />\n  <key attr.name=\"countrycode\" attr.type=\"string\" for=\"node\" id=\"\
+    d3\" />\n  <key attr.name=\"bandwidthdown\" attr.type=\"int\" for=\"node\" id=\"\
+    d2\" />\n  <key attr.name=\"bandwidthup\" attr.type=\"int\" for=\"node\" id=\"\
+    d1\" />\n  <key attr.name=\"ip\" attr.type=\"string\" for=\"node\" id=\"d0\" />\n\
+    \  <graph edgedefault=\"undirected\">\n    <node id=\"isp\">\n      <data key=\"\
+    d0\">0.0.0.0</data>\n      <data key=\"d1\">2251</data>\n      <data key=\"d2\"\
+    >17038</data>\n      <data key=\"d3\">US</data>\n      <data key=\"d4\">0.0</data>\n\
+    \    </node>\n    <edge source=\"isp\" target=\"isp\">\n      <data key=\"d5\"\
+    >50.0</data>\n      <data key=\"d6\">0.01</data>\n    </edge>\n  </graph>\n</graphml>\n"


+1. In addition to keeping or improving the internal formatting of the XML from the source, I think we can get rid of the \ns and escaped quotes by using a multiline string. https://yaml-multiline.info/

florian-vuillemot · 2020-05-27T21:09:55Z

I think we'd discussed creating a YAML->XML conversion tool first, so that the config can be written and maintained in YAML and then converted to shadow's current format "just in time"?

This tool could also be useful for rewriting old configs, but I think that might be getting ahead of ourselves a bit.

In the same tool or another one ?

robgjansen

Sorry for the delay. Here is some more guidance. I hope this isn't too specific, but I wanted to give you a better idea of what I was thinking to help you make progress! :)

I think I would make both the YAML->XML and XML->YAML part of the same tool. You can use sub-commands for this in argparse using the add_subparsers functions.

There would be two sub-parsers for the two sub-commands: yaml2xml and xml2yaml, each would contain 1 positional argument, the input filename, and 1 optional argument, the output filename. You would invoke the tool like this:

convert yaml2xml shadow.config.yaml
convert xml2yaml shadow.config.xml

By default, the tool would do the requested conversion, strip off the filename suffix and append the the suffix corresponding to the new format. So this:

convert yaml2xml shadow.config.yaml

would write a file called shadow.config.xml.

But if the optional output filename was given:

convert yaml2xml shadow.config.yaml --output myconf.xml

then the tool would just write the file to myconf.xml instead of doing the suffix conversion.

Some things to consider:

Be sure to check for errors, so that you gracefully handle the cases where the tool in invoked with a sub-command and file of the incorrect format (i.e., convert yaml2xml shadow.config.xml or convert xml2yaml shadow.config.yaml).
You should test some real convert yaml2xml shadow.config.yaml cases to make sure that the shadow.config.xml file that is produced actually works in Shadow. (Run a Shadow experiment with the XML config file and make sure it works correctly.)

robgjansen · 2020-06-05T16:16:26Z

I'm assuming this isn't ready yet since it's missing the yaml2xml mode that @sporksmith mentioned should be done first. Let me know if that is incorrect.

florian-vuillemot · 2020-06-05T20:33:56Z

Yes ! I will ask you for a review when this will be done :)

sporksmith

Submitting a place-holder review so I can get a clear signal when it's ready again. When it is, please use the 're-request' review button :)

florian-vuillemot · 2020-06-09T20:40:06Z

@robgjansen @sporksmith could you please give me a feedback on the example files ? I don't want to miss something before cleaning the code and re requesting a review.

sporksmith

Only reviewed the example xml and yaml files

sporksmith · 2020-06-09T21:52:10Z

src/test/tools/convert_xml_to_yaml/xml2yaml.yaml

+  - arguments: tgen.client.graphml.xml
+    plugin: tgen
+    starttime: 2
+option:


Maybe call this 'options' in case we add more

Same with 'host' and 'plugin' ?

sporksmith · 2020-06-09T21:56:12Z

src/test/tools/convert_xml_to_yaml/xml2yaml.yaml

+    starttime: 2
+option:
+  stoptime: 3600
+plugin:


Would be nice if we could match the typical order we use in xml files. In particular would be good to have the topology and the plugin definitions before the hosts, since those are both referenced from within the host process objects.

I think either hard-coding an order matching our "typical" order in the examples, or preserving the order in whatever source xml file being translated would be fine; whichever is easier.

Should be better :)

src/test/tools/convert_xml_to_yaml/yaml2xml.xml

sporksmith

Thanks, Florian! The consistent ordering is definitely an improvement.

src/test/tools/convert_xml_to_yaml/yaml2xml.xml

sporksmith · 2020-06-15T16:42:48Z

src/test/tools/convert_xml_to_yaml/xml2yaml.yaml

+        </edge>
+      </graph>
+    </graphml>
+plugin:


It's a little odd that we have list values with singular keys. e.g. plugin instead of plugins, host instead of hosts. The singular form makes sense in xml, where it's referring to a single item in that list, but is a little confusing in yaml, where it's referring to the whole list.

At the risk of making the mapping between xml and yaml a little less consistent, I'd lean towards remapping to/from the the singular/plural forms.

@robgjansen wdyt?
@florian-vuillemot how difficult would this be to do?

I agree with @sporksmith: plurals for the yaml lists makes more sense to me too.

Hum 3 choices:

Hard code each key and the plural

Add an 's' to each key automatically

Use both methods if there is a key which plural is different from adding a simple 's' at the end (process => processes)

What do you prefer ?

For the sake of simplicity and consistency, I'd lean towards just using a hard-coded explicit mapping for all of these; i.e. the first option.

Where could I find all keys to map ?

I think you got them all:

plugin -> plugins

host -> hosts

process -> processes

Also, "node" was renamed to "host", and "application" was renamed to "process", but we still technically accept "node" and "application" as valid inputs. So I suppose you should add these too:

node -> nodes

application -> applications

And if you're curious, the parsing code starting at root level is here:

shadow/src/main/core/support/configuration.c

Lines 733 to 754 in 9f7cd41

/* check for root-level elements */

if (!g_ascii_strcasecmp(elementName, "host") || !g_ascii_strcasecmp(elementName, "node")) {

/* handle the attributes directly */

*error = _parser_handleHostAttributes(parser, attributeNames, attributeValues);

/* handle internal content in a sub parser */

g_markup_parse_context_push(context, &(parser->xmlHostParser), parser);

} else if (!g_ascii_strcasecmp(elementName, "plugin")) {

*error = _parser_handlePluginAttributes(parser, attributeNames, attributeValues);

} else if (!g_ascii_strcasecmp(elementName, "kill")) {

*error = _parser_handleKillAttributes(parser, attributeNames, attributeValues);

} else if (!g_ascii_strcasecmp(elementName, "topology")) {

/* handle the attributes directly */

*error = _parser_handleTopologyAttributes(parser, attributeNames, attributeValues);

/* handle content text in a sub parser */

g_markup_parse_context_push(context, &(parser->xmlTopologyParser), parser);

} else if (!g_ascii_strcasecmp(elementName, "shadow")) {

*error = _parser_handleShadowAttributes(parser, attributeNames, attributeValues);

} else {

*error = g_error_new(G_MARKUP_ERROR, G_MARKUP_ERROR_UNKNOWN_ELEMENT,

"unknown 'root' child starting element '%s'", elementName);

}

}

I could also do:
node -> hosts
application -> processes
This could help to remove old input.

Sure, OK. But then you would have to spell out an explicit reverse mapping from YAML -> XML to make sure that "hosts" in YAML maps to "host" in XML (and not "node" in XML). So I think you will have to actually type out the YAML_TO_XML table rather than doing this:

YAML_TO_XML = {v: k for k, v in XML_TO_YAML.items()}

florian-vuillemot · 2020-06-19T09:05:10Z

If the examples files look good, could you please start to review the code ? :)

robgjansen · 2020-06-19T12:39:08Z

Note that @sporksmith will review this but probably won't be able to until next week.

sporksmith

Looking good! Could you add a test? Just validating that your example files get converted to the expected result is probably sufficient.

I'd put the test in the same directory as the tool; you might need to tweak some CMakeLists.txt files for it to get picked up in the automated testing.

src/tools/convert.py

florian-vuillemot · 2020-06-23T12:07:47Z

Looking good! Could you add a test? Just validating that your example files get converted to the expected result is probably sufficient.

I'd put the test in the same directory as the tool; you might need to tweak some CMakeLists.txt files for it to get picked up in the automated testing.

…vertor

… executable

…ach plateform

robgjansen

This is great! Thanks for sticking with us and getting this passing on all of our supported platforms! 😄

florian-vuillemot force-pushed the feature/convert_xml_file_to_yaml branch from d8a919c to 73df065 Compare May 25, 2020 16:56

robgjansen reviewed May 26, 2020

View reviewed changes

sporksmith reviewed May 26, 2020

View reviewed changes

florian-vuillemot requested review from robgjansen and sporksmith May 29, 2020 11:22

robgjansen requested changes May 29, 2020

View reviewed changes

robgjansen added Component: Tools Peripheral tools like parsing log files or visualizing results Status: In Progress Work is currently underway Type: Enhancement New functionality or improved design labels Jun 4, 2020

sporksmith requested changes Jun 5, 2020

View reviewed changes

sporksmith requested changes Jun 9, 2020

View reviewed changes

florian-vuillemot requested a review from sporksmith June 13, 2020 07:55

sporksmith requested changes Jun 15, 2020

View reviewed changes

florian-vuillemot requested review from robgjansen and sporksmith June 18, 2020 20:59

robgjansen removed their request for review June 19, 2020 12:39

sporksmith requested changes Jun 22, 2020

View reviewed changes

src/tools/convert.py Show resolved Hide resolved

src/tools/convert.py Show resolved Hide resolved

src/tools/convert.py Show resolved Hide resolved

florian-vuillemot closed this Jun 23, 2020

Vuillemot Florian added 23 commits July 10, 2020 16:58

Update XML to YAML file example

0cc6416

Add comments and typing

2bc3f0e

Add legacy XML tag in the XML to YAML convertion

c0d8e33

Ensure convertion operation choice is yaml2xml or xml2yaml

f2ab81e

Add test on config

040e2fb

Allow backward compatibility of PyYaml

bb5ee37

Fix typo error and remove useless /usr/bin/env in test config CMakeList

dd836b1

Change the Argument Parser description in the shadow XML and YAML con…

bc429db

…vertor

Update the example of the shadow config convertor XML and YAML

f9c7604

Remove all logic from the test config executable

b8c1fb1

Update tests on shadow XML and YAML convertor and add tests with diff…

5b7ee22

… executable

Install PyYaml dependancies in GitHub action

a176e2c

Add dependencies between config diff and XML/YAML convertor

a40b367

Add the installation of pip3 on each target host

2e8fa8e

Remove typo in debian packages

6dcc998

Use pip3 and not pip in PyYaml installation

bd471fd

Ordering attribut element from YAML to XML to obtain same result on e…

d575887

…ach plateform

Allow to run with previous version of Python3.6

d9a25ef

Fix naming error on convert XML/YAML shadow tool CMakelist

7266c98

Try to install python with action setup-python V2

fda4f35

Change pip installation

5bd4138

Add Python3-pip and force minimun of python3.6 in container

ec96947

Move python package install in dependencies shell script

dca6326

florian-vuillemot force-pushed the feature/convert_xml_file_to_yaml branch from 6a8d8ac to dca6326 Compare July 10, 2020 14:58

florian-vuillemot requested a review from sporksmith July 10, 2020 15:06

robgjansen approved these changes Jul 10, 2020

View reviewed changes

robgjansen assigned sporksmith Jul 10, 2020

sporksmith merged commit e950797 into shadow:dev Jul 10, 2020

florian-vuillemot deleted the feature/convert_xml_file_to_yaml branch July 10, 2020 22:45

robgjansen mentioned this pull request Jul 10, 2020

More human-readable Shadow config #773

Closed

5 tasks

	/* check for root-level elements */
	if (!g_ascii_strcasecmp(elementName, "host") \|\| !g_ascii_strcasecmp(elementName, "node")) {
	/* handle the attributes directly */
	*error = _parser_handleHostAttributes(parser, attributeNames, attributeValues);
	/* handle internal content in a sub parser */
	g_markup_parse_context_push(context, &(parser->xmlHostParser), parser);
	} else if (!g_ascii_strcasecmp(elementName, "plugin")) {
	*error = _parser_handlePluginAttributes(parser, attributeNames, attributeValues);
	} else if (!g_ascii_strcasecmp(elementName, "kill")) {
	*error = _parser_handleKillAttributes(parser, attributeNames, attributeValues);
	} else if (!g_ascii_strcasecmp(elementName, "topology")) {
	/* handle the attributes directly */
	*error = _parser_handleTopologyAttributes(parser, attributeNames, attributeValues);
	/* handle content text in a sub parser */
	g_markup_parse_context_push(context, &(parser->xmlTopologyParser), parser);
	} else if (!g_ascii_strcasecmp(elementName, "shadow")) {
	*error = _parser_handleShadowAttributes(parser, attributeNames, attributeValues);
	} else {
	*error = g_error_new(G_MARKUP_ERROR, G_MARKUP_ERROR_UNKNOWN_ELEMENT,
	"unknown 'root' child starting element '%s'", elementName);
	}
	}

Conversation

florian-vuillemot commented May 20, 2020

Uh oh!

florian-vuillemot commented May 20, 2020

Uh oh!

robgjansen commented May 21, 2020

Uh oh!

robgjansen commented May 21, 2020

Uh oh!

sporksmith commented May 21, 2020

Uh oh!

florian-vuillemot commented May 21, 2020

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!

sporksmith 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!

florian-vuillemot commented May 27, 2020

Uh oh!

robgjansen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robgjansen commented Jun 5, 2020

Uh oh!

florian-vuillemot commented Jun 5, 2020

Uh oh!

sporksmith left a comment

Choose a reason for hiding this comment

Uh oh!

florian-vuillemot commented Jun 9, 2020

Uh oh!

sporksmith 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!

Uh oh!

sporksmith left a comment

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!

robgjansen left a comment •

edited

Loading

robgjansen Jun 18, 2020 •

edited

Loading