Re: Review Request 34300: Do better sanitation on the client side when encountering unbound pystachio refs
On June 4, 2015, 4:55 p.m., Kevin Sweeney wrote: src/main/python/apache/aurora/config/thrift.py, line 218 https://reviews.apache.org/r/34300/diff/3/?file=975774#file975774line218 Why set this at all? Does the scheduler read this field? Brian Wickman wrote: it is not. i will leave it up to AURORA-739 to remove support entirely (or add it back in a sensible way.) In that case, please drop this set from the client-side. - Kevin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34300/#review86742 --- On June 1, 2015, 11:05 a.m., Brian Wickman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34300/ --- (Updated June 1, 2015, 11:05 a.m.) Review request for Aurora and Kevin Sweeney. Bugs: AURORA-739 https://issues.apache.org/jira/browse/AURORA-739 Repository: aurora Description --- It's possible to define nested refs that can cause the executor to stack trace, e.g. {{derp[{{thermos.ports[http]}}]}} is perfectly valid but crashes the executor. Diffs - src/main/python/apache/aurora/config/__init__.py dd2f89014a3da730364b14e01c499ac0f2c288c1 src/main/python/apache/aurora/config/schema/base.py a87524a8b3ad5aa0e337e0a0028cecb85865b4e6 src/main/python/apache/aurora/config/thrift.py 810febb637d168b07c4aea77984e1d1451a39af2 src/main/python/apache/aurora/executor/common/task_info.py d110faf08135d94d9af95ad74613950c56248c09 src/main/python/apache/thermos/config/dsl.py 0663a9ad8ae194b63ce6d77dabfe65564e2d26dd src/main/python/apache/thermos/config/loader.py d77ab9a52b16e9d65acdb95f01fd251ae8ab2b6e src/test/python/apache/aurora/client/test_config.py c56779712b91f621261358aa7ebd6c4fc65446a0 src/test/python/apache/aurora/config/test_thrift.py 654c0b5ae82c98db163c7a44301ff6b23e19b211 src/test/python/apache/aurora/executor/common/test_task_info.py 102ba531aa6c28f2d74bd0d7f1668e5861e3a6b8 Diff: https://reviews.apache.org/r/34300/diff/ Testing --- Added some regression tests. Thanks, Brian Wickman
Re: Review Request 34300: Do better sanitation on the client side when encountering unbound pystachio refs
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34300/#review86742 --- Ship it! src/main/python/apache/aurora/config/thrift.py https://reviews.apache.org/r/34300/#comment138805 Why set this at all? Does the scheduler read this field? - Kevin Sweeney On June 1, 2015, 11:05 a.m., Brian Wickman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34300/ --- (Updated June 1, 2015, 11:05 a.m.) Review request for Aurora and Kevin Sweeney. Bugs: AURORA-739 https://issues.apache.org/jira/browse/AURORA-739 Repository: aurora Description --- It's possible to define nested refs that can cause the executor to stack trace, e.g. {{derp[{{thermos.ports[http]}}]}} is perfectly valid but crashes the executor. Diffs - src/main/python/apache/aurora/config/__init__.py dd2f89014a3da730364b14e01c499ac0f2c288c1 src/main/python/apache/aurora/config/schema/base.py a87524a8b3ad5aa0e337e0a0028cecb85865b4e6 src/main/python/apache/aurora/config/thrift.py 810febb637d168b07c4aea77984e1d1451a39af2 src/main/python/apache/aurora/executor/common/task_info.py d110faf08135d94d9af95ad74613950c56248c09 src/main/python/apache/thermos/config/dsl.py 0663a9ad8ae194b63ce6d77dabfe65564e2d26dd src/main/python/apache/thermos/config/loader.py d77ab9a52b16e9d65acdb95f01fd251ae8ab2b6e src/test/python/apache/aurora/client/test_config.py c56779712b91f621261358aa7ebd6c4fc65446a0 src/test/python/apache/aurora/config/test_thrift.py 654c0b5ae82c98db163c7a44301ff6b23e19b211 src/test/python/apache/aurora/executor/common/test_task_info.py 102ba531aa6c28f2d74bd0d7f1668e5861e3a6b8 Diff: https://reviews.apache.org/r/34300/diff/ Testing --- Added some regression tests. Thanks, Brian Wickman
Re: Review Request 34300: Do better sanitation on the client side when encountering unbound pystachio refs
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34300/#review86028 --- Ship it! Master (827b9ab) is green with this patch. ./build-support/jenkins/build.sh I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On June 1, 2015, 6:05 p.m., Brian Wickman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34300/ --- (Updated June 1, 2015, 6:05 p.m.) Review request for Aurora and Kevin Sweeney. Bugs: AURORA-739 https://issues.apache.org/jira/browse/AURORA-739 Repository: aurora Description --- It's possible to define nested refs that can cause the executor to stack trace, e.g. {{derp[{{thermos.ports[http]}}]}} is perfectly valid but crashes the executor. Diffs - src/main/python/apache/aurora/config/__init__.py dd2f89014a3da730364b14e01c499ac0f2c288c1 src/main/python/apache/aurora/config/schema/base.py a87524a8b3ad5aa0e337e0a0028cecb85865b4e6 src/main/python/apache/aurora/config/thrift.py 810febb637d168b07c4aea77984e1d1451a39af2 src/main/python/apache/aurora/executor/common/task_info.py d110faf08135d94d9af95ad74613950c56248c09 src/main/python/apache/thermos/config/dsl.py 0663a9ad8ae194b63ce6d77dabfe65564e2d26dd src/main/python/apache/thermos/config/loader.py d77ab9a52b16e9d65acdb95f01fd251ae8ab2b6e src/test/python/apache/aurora/client/test_config.py c56779712b91f621261358aa7ebd6c4fc65446a0 src/test/python/apache/aurora/config/test_thrift.py 654c0b5ae82c98db163c7a44301ff6b23e19b211 src/test/python/apache/aurora/executor/common/test_task_info.py 102ba531aa6c28f2d74bd0d7f1668e5861e3a6b8 Diff: https://reviews.apache.org/r/34300/diff/ Testing --- Added some regression tests. Thanks, Brian Wickman
Re: Review Request 34300: Do better sanitation on the client side when encountering unbound pystachio refs
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34300/ --- (Updated June 1, 2015, 6:05 p.m.) Review request for Aurora and Kevin Sweeney. Changes --- Fix checkstyle violation. Bugs: AURORA-739 https://issues.apache.org/jira/browse/AURORA-739 Repository: aurora Description --- It's possible to define nested refs that can cause the executor to stack trace, e.g. {{derp[{{thermos.ports[http]}}]}} is perfectly valid but crashes the executor. Diffs (updated) - src/main/python/apache/aurora/config/__init__.py dd2f89014a3da730364b14e01c499ac0f2c288c1 src/main/python/apache/aurora/config/schema/base.py a87524a8b3ad5aa0e337e0a0028cecb85865b4e6 src/main/python/apache/aurora/config/thrift.py 810febb637d168b07c4aea77984e1d1451a39af2 src/main/python/apache/aurora/executor/common/task_info.py d110faf08135d94d9af95ad74613950c56248c09 src/main/python/apache/thermos/config/dsl.py 0663a9ad8ae194b63ce6d77dabfe65564e2d26dd src/main/python/apache/thermos/config/loader.py d77ab9a52b16e9d65acdb95f01fd251ae8ab2b6e src/test/python/apache/aurora/client/test_config.py c56779712b91f621261358aa7ebd6c4fc65446a0 src/test/python/apache/aurora/config/test_thrift.py 654c0b5ae82c98db163c7a44301ff6b23e19b211 src/test/python/apache/aurora/executor/common/test_task_info.py 102ba531aa6c28f2d74bd0d7f1668e5861e3a6b8 Diff: https://reviews.apache.org/r/34300/diff/ Testing --- Added some regression tests. Thanks, Brian Wickman
Re: Review Request 34300: Do better sanitation on the client side when encountering unbound pystachio refs
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34300/#review85258 --- Master (6db13ba) is red with this patch. ./build-support/jenkins/build.sh Using cached twitter.common.collections-0.3.0.tar.gz Collecting smmap=0.8.5 (from gitdb=0.5.1-GitPython==0.3.2.RC1-twitter.checkstyle==0.1.0) Using cached smmap-0.9.0.tar.gz Collecting twitter.common.string==0.3.0 (from twitter.common.process==0.3.0-twitter.common.app==0.3.0-twitter.checkstyle==0.1.0) Using cached twitter.common.string-0.3.0.tar.gz Collecting twitter.common.options==0.3.0 (from twitter.common.log==0.3.0-twitter.common.app==0.3.0-twitter.checkstyle==0.1.0) Using cached twitter.common.options-0.3.0.tar.gz Collecting twitter.common.dirutil==0.3.0 (from twitter.common.log==0.3.0-twitter.common.app==0.3.0-twitter.checkstyle==0.1.0) Using cached twitter.common.dirutil-0.3.0.tar.gz Collecting twitter.common.contextutil==0.3.0 (from twitter.common.util==0.3.0-twitter.common.app==0.3.0-twitter.checkstyle==0.1.0) Using cached twitter.common.contextutil-0.3.0.tar.gz Collecting twitter.common.lang==0.3.0 (from twitter.common.collections==0.3.0-twitter.common.app==0.3.0-twitter.checkstyle==0.1.0) Using cached twitter.common.lang-0.3.0.tar.gz Installing collected packages: pyflakes, pep8, smmap, gitdb, GitPython, twitter.common.lang, twitter.common.string, twitter.common.process, twitter.common.options, twitter.common.dirutil, twitter.common.log, twitter.common.contextutil, twitter.common.util, twitter.common.collections, twitter.common.app, twitter.checkstyle Running setup.py install for pyflakes Running setup.py install for pep8 Running setup.py install for smmap Running setup.py install for gitdb Running setup.py install for GitPython Running setup.py install for twitter.common.lang Running setup.py install for twitter.common.string Running setup.py install for twitter.common.process Running setup.py install for twitter.common.options Running setup.py install for twitter.common.dirutil Running setup.py install for twitter.common.log Running setup.py install for twitter.common.contextutil Running setup.py install for twitter.common.util Running setup.py install for twitter.common.collections Running setup.py install for twitter.common.app Running setup.py install for twitter.checkstyle Successfully installed GitPython-0.3.2rc1 gitdb-0.6.4 pep8-1.4.5 pyflakes-0.7.2 smmap-0.9.0 twitter.checkstyle-0.1.0 twitter.common.app-0.3.0 twitter.common.collections-0.3.0 twitter.common.contextutil-0.3.0 twitter.common.dirutil-0.3.0 twitter.common.lang-0.3.0 twitter.common.log-0.3.0 twitter.common.options-0.3.0 twitter.common.process-0.3.0 twitter.common.string-0.3.0 twitter.common.util-0.3.0 F401:ERROR src/test/python/apache/aurora/client/test_config.py:019 'temporary_file' imported but unused |from twitter.common.contextutil import temporary_dir, temporary_file F401:ERROR src/test/python/apache/aurora/config/test_thrift.py:019 'Map' imported but unused |from pystachio import Map, String F401:ERROR src/test/python/apache/aurora/config/test_thrift.py:019 'String' imported but unused |from pystachio import Map, String I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On May 26, 2015, 9:03 p.m., Brian Wickman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34300/ --- (Updated May 26, 2015, 9:03 p.m.) Review request for Aurora and Kevin Sweeney. Bugs: AURORA-739 https://issues.apache.org/jira/browse/AURORA-739 Repository: aurora Description --- It's possible to define nested refs that can cause the executor to stack trace, e.g. {{derp[{{thermos.ports[http]}}]}} is perfectly valid but crashes the executor. Diffs - src/main/python/apache/aurora/config/__init__.py dd2f89014a3da730364b14e01c499ac0f2c288c1 src/main/python/apache/aurora/config/schema/base.py a87524a8b3ad5aa0e337e0a0028cecb85865b4e6 src/main/python/apache/aurora/config/thrift.py 810febb637d168b07c4aea77984e1d1451a39af2 src/main/python/apache/aurora/executor/common/task_info.py d110faf08135d94d9af95ad74613950c56248c09 src/main/python/apache/thermos/config/dsl.py 0663a9ad8ae194b63ce6d77dabfe65564e2d26dd src/main/python/apache/thermos/config/loader.py d77ab9a52b16e9d65acdb95f01fd251ae8ab2b6e src/test/python/apache/aurora/client/test_config.py c56779712b91f621261358aa7ebd6c4fc65446a0 src/test/python/apache/aurora/config/test_thrift.py 654c0b5ae82c98db163c7a44301ff6b23e19b211 src/test/python/apache/aurora/executor/common/test_task_info.py
Re: Review Request 34300: Do better sanitation on the client side when encountering unbound pystachio refs
On May 16, 2015, 1:17 a.m., Kevin Sweeney wrote: src/main/python/apache/aurora/config/schema/base.py, line 98 https://reviews.apache.org/r/34300/diff/1/?file=961840#file961840line98 Would be nice to have a wrapper here like Deprecated(Map(String, String)) You filed https://github.com/wickman/pystachio/issues/9 over two years ago and you're still probably right ;) On May 16, 2015, 1:17 a.m., Kevin Sweeney wrote: src/main/python/apache/aurora/config/__init__.py, lines 255-257 https://reviews.apache.org/r/34300/diff/1/?file=961839#file961839line255 Delete this property entirely rather than return a dummy value? Presumably anything accessing still accessing it should AttributeError. Zameer Manji wrote: +1, this is something that can be deleted because it only deals with our code and not configs. Killed. On May 16, 2015, 1:17 a.m., Kevin Sweeney wrote: src/main/python/apache/aurora/config/thrift.py, lines 97-109 https://reviews.apache.org/r/34300/diff/1/?file=961841#file961841line97 Is this still necessary and/or buggy? Reading the comment it seems this should have prevented seeing this bug. Not buggy, just insufficient. We allow *some* unbound refs -- specifically {{thermos.ports}}, {{mesos.task_id}}, and {{mesos.instance}}. The problem is that if you do something like {{array[{{mesos.instance}}]}}, the only unbound ref that pystachio sees is {{mesos.instance}} which it's willing to accept. Once {{mesos.instance}} is bound (in the executor) then we know that {{array[123]}} is actually an unbound ref. The approach taken in this review is the simplest way (I could think of at least) to attempt to catch these iteratively unbound refs, by mimicking what the executor would do but instead on the client side. On May 16, 2015, 1:17 a.m., Kevin Sweeney wrote: src/main/python/apache/thermos/config/loader.py, lines 168-170 https://reviews.apache.org/r/34300/diff/1/?file=961844#file961844line168 This seems like a DRY violation - how do we ensure that any new fields provided in the thermos namespace will be bound here? Could we iterate over the available field names here instead? There are TODOs in the code to move away from pystachio-binding in the executor and instead doing %%-%% binding a la Aurora. This way we just bind {{mesos.instance}} to %%instance%%, {{mesos.task_id}} to %%task_id%% and {{thermos.ports[foo]}} to %%ports[foo]%%. This means that we can definitely detect unbound refs since we'd require everything to be bound in the client. We'd then remove pystachio support from the Thermos runner altogether and have it do simple string replacement instead. Does this seem reasonable? On May 16, 2015, 1:17 a.m., Kevin Sweeney wrote: src/test/python/apache/aurora/client/test_config.py, lines 81-85 https://reviews.apache.org/r/34300/diff/1/?file=961845#file961845line81 Seems like there's no reason to write the config out to disk here versus using a BytesIO, no? Was just cargo culting. Will use BytesIO instead. On May 16, 2015, 1:17 a.m., Kevin Sweeney wrote: src/test/python/apache/aurora/client/test_config.py, lines 93-98 https://reviews.apache.org/r/34300/diff/1/?file=961845#file961845line93 Same here - can we use a BytesIO instead of an actual file? Done On May 16, 2015, 1:17 a.m., Kevin Sweeney wrote: src/test/python/apache/aurora/config/test_thrift.py, line 148 https://reviews.apache.org/r/34300/diff/1/?file=961846#file961846line148 Drop this call entirely? Production code shouldn't reference .task_links anymore Done On May 16, 2015, 1:17 a.m., Kevin Sweeney wrote: src/test/python/apache/aurora/executor/common/test_task_info.py, line 53 https://reviews.apache.org/r/34300/diff/1/?file=961847#file961847line53 Rather than asserting on the contents of the error message, consider defining a custom exception type ``` class UnexpectedUnboundRefs(ValueError): pass ``` and expecting that exception type. done On May 16, 2015, 1:17 a.m., Kevin Sweeney wrote: src/test/python/apache/aurora/executor/common/test_task_info.py, line 73 https://reviews.apache.org/r/34300/diff/1/?file=961847#file961847line73 Consider expecting a specialized error type here as well. done - Brian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34300/#review84010 --- On May 16, 2015, 12:01 a.m., Brian Wickman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34300/ --- (Updated May 16, 2015, 12:01 a.m.) Review request for Aurora and Kevin Sweeney. Repository: aurora
Re: Review Request 34300: Do better sanitation on the client side when encountering unbound pystachio refs
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34300/ --- (Updated May 26, 2015, 9:03 p.m.) Review request for Aurora and Kevin Sweeney. Changes --- Address Kevin's review feedback. Bugs: AURORA-739 https://issues.apache.org/jira/browse/AURORA-739 Repository: aurora Description --- It's possible to define nested refs that can cause the executor to stack trace, e.g. {{derp[{{thermos.ports[http]}}]}} is perfectly valid but crashes the executor. Diffs (updated) - src/main/python/apache/aurora/config/__init__.py dd2f89014a3da730364b14e01c499ac0f2c288c1 src/main/python/apache/aurora/config/schema/base.py a87524a8b3ad5aa0e337e0a0028cecb85865b4e6 src/main/python/apache/aurora/config/thrift.py 810febb637d168b07c4aea77984e1d1451a39af2 src/main/python/apache/aurora/executor/common/task_info.py d110faf08135d94d9af95ad74613950c56248c09 src/main/python/apache/thermos/config/dsl.py 0663a9ad8ae194b63ce6d77dabfe65564e2d26dd src/main/python/apache/thermos/config/loader.py d77ab9a52b16e9d65acdb95f01fd251ae8ab2b6e src/test/python/apache/aurora/client/test_config.py c56779712b91f621261358aa7ebd6c4fc65446a0 src/test/python/apache/aurora/config/test_thrift.py 654c0b5ae82c98db163c7a44301ff6b23e19b211 src/test/python/apache/aurora/executor/common/test_task_info.py 102ba531aa6c28f2d74bd0d7f1668e5861e3a6b8 Diff: https://reviews.apache.org/r/34300/diff/ Testing --- Added some regression tests. Thanks, Brian Wickman
Review Request 34300: Do better sanitation on the client side when encountering unbound pystachio refs
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34300/ --- Review request for Aurora and Kevin Sweeney. Repository: aurora Description --- It's possible to define nested refs that can cause the executor to stack trace, e.g. {{derp[{{thermos.ports[http]}}]}} is perfectly valid but crashes the executor. Diffs - src/main/python/apache/aurora/config/__init__.py dd2f89014a3da730364b14e01c499ac0f2c288c1 src/main/python/apache/aurora/config/schema/base.py a87524a8b3ad5aa0e337e0a0028cecb85865b4e6 src/main/python/apache/aurora/config/thrift.py 810febb637d168b07c4aea77984e1d1451a39af2 src/main/python/apache/aurora/executor/common/task_info.py d110faf08135d94d9af95ad74613950c56248c09 src/main/python/apache/thermos/config/dsl.py 0663a9ad8ae194b63ce6d77dabfe65564e2d26dd src/main/python/apache/thermos/config/loader.py d77ab9a52b16e9d65acdb95f01fd251ae8ab2b6e src/test/python/apache/aurora/client/test_config.py c56779712b91f621261358aa7ebd6c4fc65446a0 src/test/python/apache/aurora/config/test_thrift.py 654c0b5ae82c98db163c7a44301ff6b23e19b211 src/test/python/apache/aurora/executor/common/test_task_info.py 102ba531aa6c28f2d74bd0d7f1668e5861e3a6b8 Diff: https://reviews.apache.org/r/34300/diff/ Testing --- Added some regression tests. Thanks, Brian Wickman