Make ValidationPool accepts execution mode to run custom command or in process validation #1622

cecton · 2020-08-21T13:34:51Z

Related to paritytech/cumulus#199
Also related to paritytech/cumulus#201 for the actual use

~~This change will make the polkadot test service use the "test mode" of the validation pool.~~

I ~~also~~ moved the test flag from the validate_candidate to the pool directly which looks like a refactoring but my actual goal ~~was~~ is to pass the test mode all the way down to the Validation pool. This will be done for v1 of the validation.

Forked at: bf7ccb8 Parent branch: origin/master

validation/src/validation_service/mod.rs

bkchr · 2020-08-21T14:05:47Z

validation/src/validation_service/mod.rs

@@ -132,6 +132,9 @@ pub struct ServiceBuilder<C, N, P, SC, SP> {
 	pub keystore: KeyStorePtr,
 	/// The maximum block-data size in bytes.
 	pub max_block_data_size: Option<u64>,
+	/// The validation worker is called using the subcommand `--nocapture validation_worker` instead
+	/// of `validation-worker`, suitable for test environment.
+	pub test_mode: bool,


Why don't we pass an enum here:

enum ValidationExecutionMode { Local, Remote { binary: Path, args: Vec<String>, } }

Fair point!

I don't know if we need that much flexibility 🤔 do you think? Nobody touched this except me recently and only for testing.

Well! I don't mind changing it for an enum! Should I proceed with the changes?

Yes, this makes this parameter also more clear on what it does.

Done 8a5aa08

Forked at: bf7ccb8 Parent branch: origin/master

bkchr · 2020-08-24T14:13:48Z

service/src/lib.rs

@@ -391,6 +393,15 @@ pub fn new_full<RuntimeApi, Executor>(

 		polkadot_network_service.register_availability_store(availability_store.clone());

+		let execution_mode = if test_mode {


Suggested change

let execution_mode = if test_mode {

let execution_mode = if !test_mode {

Wait what. Why? The whole point was to get this on the worker args for the tests. Am I missing something?

See my other comment, this makes no sense. In test mode you don't want to start the external worker and do it as thread instead. You mixed the concepts here totally. You should read the code of the validation again that decides on whether to start a validation worker or to run it inside the process.

Oh no it's fine I get it now!

So Local would be use by the integration test on cumulus because we don't need it to be interruptible (or any more sophisticated), just need it to run for a few seconds during the test and everything is stopped anyway (because tests have their own process in Rust which is great I think).

Remote is the currently supported case in polkadot (and polkadot's tests!) where a background tasks is ran by spawning a new child process.

ez pz

service/src/lib.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

Parent branch: origin/master Forked at: bf7ccb8

bkchr · 2020-08-25T14:14:15Z

parachain/src/wasm_executor/validation_host.rs

+		let (cmd, args) = match execution_mode {
+			ValidationExecutionMode::Local => (
+				env::current_exe()?,
+				WORKER_ARGS.iter().map(|x| x.to_string()).collect()
+			),
+			ValidationExecutionMode::Remote { binary, args } => (binary, args),
+		};


Wait. You clear did not understand what Local and Remote mean here.

Local => Run the verification inside the current process
Remote => Use the given external command and args to start a worker.

I actually would prefer an enum option to use default args and executable name. Defaults should work in all cases other than testing, and requiring the user to always specify executable/args would be bad API design. It is very much an internal implementation details and it only leaks away because tests require special handling.

What you mean by enum option?

An enum value that does not require setting executable name for out-of-process validation. E.g. RemoteDefault or simply Remote. Basically something like this:

pub enum ValidationExecutionMode { InProcess, ExternalProcessSelfHost, ExternalProcessCustomHost { binary: PathBuf, args: Vec<String>, },

Okay. Sounds reasonable.

bkchr · 2020-08-25T14:16:31Z

service/src/lib.rs

@@ -391,6 +393,15 @@ pub fn new_full<RuntimeApi, Executor>(

 		polkadot_network_service.register_availability_store(availability_store.clone());

+		let execution_mode = if test_mode {


See my other comment, this makes no sense. In test mode you don't want to start the external worker and do it as thread instead. You mixed the concepts here totally. You should read the code of the validation again that decides on whether to start a validation worker or to run it inside the process.

arkpar · 2020-08-25T15:24:27Z

See my other comment, this makes no sense. In test mode you don't want to start the external worker and do it as thread instead.

Some tests rely on the interruptable execution support. I would prefer all test to run as close to the real world usage as possible. The current version of this PR supports out of process execution for tests and I see no problem with that.

bkchr · 2020-08-25T20:25:07Z

See my other comment, this makes no sense. In test mode you don't want to start the external worker and do it as thread instead.

Some tests rely on the interruptable execution support. I would prefer all test to run as close to the real world usage as possible. The current version of this PR supports out of process execution for tests and I see no problem with that.

Yeah and that is totally find and is still supported. I also don't want to remove support for this. However, @cecton had the problem that she wrote an integration tests and was not aware of the remote test execution. And this lead to some nested loop of always starting the same process over and over again.

For our integration test usage it is totally fine to run the validation in the process and that is what this pr is about.

So, what I imagined is the following:

You can specify if you want to run the validation worker "local" aka in the process or "remote" aka as a new process
This is configured when setting up the validation pool.
We can make the internals easier that used Local, Remote and RemoteTest, by merging the later ones. So, when specifying the Remote we give the path to the executable and the arguments.

arkpar · 2020-08-26T07:11:19Z

Yeah and that is totally find and is still supported. I also don't want to remove support for this. However, @cecton had the problem that she wrote an integration tests and was not aware of the remote test execution. And this lead to some nested loop of always starting the same process over and over again.

Right, but this PR fixes that. It allows for tests to run in external process.

For our integration test usage it is totally fine to run the validation in the process and that is what this pr is about.

Are you sure about that? This might not be important for unit tests, but integration tests should be as close to real life behaviour as possible.

bkchr · 2020-08-26T08:48:18Z

Yeah I know, but it is also weird to replicate always this extra test that is being called as the worker. The worker only exists to make validation interruptable and more observabale. I'm not that convinced that this feature is that important to test.

cecton · 2020-08-30T05:42:54Z

For our integration test usage it is totally fine to run the validation in the process and that is what this pr is about.

Are you sure about that? This might not be important for unit tests, but integration tests should be as close to real life behaviour as possible.

Yes I know it sounds weird but as far as cumulus is concerned we don't really test that polkadot can run its validation in a subprocess properly. That is a test done in polkadot. We trust that polkadot is working and is tested. In cumulus we only care that both chains interact properly. Thus the simplest (and with less pitfalls) implementation is better.

However, @cecton had the problem that she wrote an integration tests and was not aware of the remote test execution.

No no I was very aware of that. I did made the subcommand for the original integration tests. I just forgot about it and this time no error message warned me of my mistake (1). What I mean is: this is a human mistake and this is a pitfall.

(1) in the original integration test the command failed when received arguments it couldn't parse. Here it's the test binary that is ran and it doesn't fail easily, it tries to find a test called "validation-worker", filters out everything, and exit with a status 0. This is pure evil.

Parent branch: origin/master Forked at: bf7ccb8

Forked at: bf7ccb8 Parent branch: origin/master

cecton · 2020-09-02T09:44:59Z

@bkchr @arkpar ok this is ready for review

…n process validation (#1622) Squashed commit of the following: commit 3eec1ef Author: Cecile Tonglet <cecile@parity.io> Date: Wed Sep 2 11:43:47 2020 +0200 CLEANUP Forked at: bf7ccb8 Parent branch: origin/master commit 50ba484 Author: Cecile Tonglet <cecile@parity.io> Date: Wed Sep 2 11:42:07 2020 +0200 Add test commit 33ced08 Author: Cecile Tonglet <cecile@parity.io> Date: Wed Sep 2 11:21:57 2020 +0200 Run validation inside the same process commit 4e113ba Author: Cecile Tonglet <cecile@parity.io> Date: Wed Sep 2 10:25:06 2020 +0200 Adapt code to review suggestions commit f46e7a7 Merge: 6301515 e2ebf25 Author: Cecile Tonglet <cecile@parity.io> Date: Wed Sep 2 10:03:04 2020 +0200 Merge commit e2ebf25 (no conflict) Parent branch: origin/master Forked at: bf7ccb8 commit 6301515 Merge: 7f57373 01ffc66 Author: Cecile Tonglet <cecile@parity.io> Date: Tue Aug 25 01:09:27 2020 +0200 Merge commit 01ffc66 (no conflict) Parent branch: origin/master Forked at: bf7ccb8 commit 7f57373 Merge: 1fd2987 00202d3 Author: Cecile Tonglet <cecile@parity.io> Date: Tue Aug 25 01:09:22 2020 +0200 Merge commit 00202d3 (conflicts) Parent branch: origin/master Forked at: bf7ccb8 commit 1fd2987 Merge: 41cb5b5 62cf5b9 Author: Cecile Tonglet <cecile@parity.io> Date: Tue Aug 25 01:05:23 2020 +0200 Merge commit 62cf5b9 (no conflict) Parent branch: origin/master Forked at: bf7ccb8 commit 41cb5b5 Author: Cecile Tonglet <cecile@parity.io> Date: Tue Aug 25 01:03:31 2020 +0200 Update service/src/lib.rs Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> commit 8a5aa08 Author: Cecile Tonglet <cecile@parity.io> Date: Fri Aug 21 19:06:36 2020 +0200 WIP Forked at: bf7ccb8 Parent branch: origin/master commit b7a97df Author: Cecile Tonglet <cecile@parity.io> Date: Fri Aug 21 15:56:21 2020 +0200 Fix test commit e28eed7 Author: Cecile Tonglet <cecile@parity.io> Date: Fri Aug 21 15:37:13 2020 +0200 Update validation/src/validation_service/mod.rs commit 8c880f7 Author: Cecile Tonglet <cecile@parity.io> Date: Fri Aug 21 15:02:45 2020 +0200 Propagate test mode all the way down to ValidationPool commit d32b42a Author: Cecile Tonglet <cecile@parity.io> Date: Fri Aug 21 15:27:56 2020 +0200 Initial commit Forked at: bf7ccb8 Parent branch: origin/master

cecton · 2020-09-09T10:17:17Z

bot merge

ghost · 2020-09-09T10:17:21Z

Trying merge.

…mmand or in process validation (#1622) (#1675)

cecton added 2 commits August 21, 2020 15:27

Initial commit

d32b42a

Forked at: bf7ccb8 Parent branch: origin/master

Propagate test mode all the way down to ValidationPool

8c880f7

cecton added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Aug 21, 2020

cecton requested review from bkchr, arkpar and rphmeier August 21, 2020 13:34

cecton commented Aug 21, 2020

View reviewed changes

validation/src/validation_service/mod.rs Outdated Show resolved Hide resolved

cecton added 2 commits August 21, 2020 15:37

Update validation/src/validation_service/mod.rs

e28eed7

Fix test

b7a97df

bkchr reviewed Aug 21, 2020

View reviewed changes

WIP

8a5aa08

Forked at: bf7ccb8 Parent branch: origin/master

arkpar approved these changes Aug 24, 2020

View reviewed changes

bkchr approved these changes Aug 24, 2020

View reviewed changes

cecton and others added 4 commits August 25, 2020 01:03

Update service/src/lib.rs

41cb5b5

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

Merge commit 62cf5b9 (no conflict)

1fd2987

Parent branch: origin/master Forked at: bf7ccb8

Merge commit 00202d3 (conflicts)

7f57373

Parent branch: origin/master Forked at: bf7ccb8

Merge commit 01ffc66 (no conflict)

6301515

Parent branch: origin/master Forked at: bf7ccb8

bkchr suggested changes Aug 25, 2020

View reviewed changes

cecton added 2 commits September 2, 2020 10:03

Merge commit e2ebf25 (no conflict)

f46e7a7

Parent branch: origin/master Forked at: bf7ccb8

Adapt code to review suggestions

4e113ba

cecton changed the title ~~Propagate test mode all the way down to ValidationPool~~ Make ValidationPool accepts execution mode to run custom command or in process validation Sep 2, 2020

cecton added 2 commits September 2, 2020 11:21

Run validation inside the same process

33ced08

Add test

50ba484

CLEANUP

3eec1ef

Forked at: bf7ccb8 Parent branch: origin/master

arkpar approved these changes Sep 2, 2020

View reviewed changes

cecton mentioned this pull request Sep 2, 2020

Backport: Make ValidationPool accepts execution mode to run custom command or in process validation (#1622) #1675

Merged

bkchr approved these changes Sep 9, 2020

View reviewed changes

ghost merged commit 61dd388 into master Sep 9, 2020

ghost deleted the cecton-test-mode-all-the-way-down branch September 9, 2020 10:17

cecton added a commit that referenced this pull request Sep 9, 2020

Backport: Make ValidationPool accepts execution mode to run custom co…

83209d4

…mmand or in process validation (#1622) (#1675)

cecton mentioned this pull request Sep 10, 2020

Validation: don't detect STDIN closing when running in process #1695

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ValidationPool accepts execution mode to run custom command or in process validation #1622

Make ValidationPool accepts execution mode to run custom command or in process validation #1622

cecton commented Aug 21, 2020 •

edited

Loading

bkchr Aug 21, 2020

cecton Aug 21, 2020

bkchr Aug 21, 2020

cecton Aug 21, 2020

bkchr Aug 24, 2020

cecton Aug 24, 2020

bkchr Aug 25, 2020

cecton Aug 26, 2020

bkchr Aug 25, 2020

arkpar Aug 25, 2020

bkchr Aug 25, 2020

arkpar Aug 26, 2020

bkchr Aug 26, 2020

bkchr Aug 25, 2020

arkpar commented Aug 25, 2020 •

edited

Loading

bkchr commented Aug 25, 2020 •

edited by arkpar

Loading

arkpar commented Aug 26, 2020

bkchr commented Aug 26, 2020

cecton commented Aug 30, 2020

cecton commented Sep 2, 2020

cecton commented Sep 9, 2020

ghost commented Sep 9, 2020

		@@ -391,6 +393,15 @@ pub fn new_full<RuntimeApi, Executor>(

		polkadot_network_service.register_availability_store(availability_store.clone());

		let execution_mode = if test_mode {

	let execution_mode = if test_mode {
	let execution_mode = if !test_mode {

Make ValidationPool accepts execution mode to run custom command or in process validation #1622

Make ValidationPool accepts execution mode to run custom command or in process validation #1622

Conversation

cecton commented Aug 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkpar commented Aug 25, 2020 • edited Loading

bkchr commented Aug 25, 2020 • edited by arkpar Loading

arkpar commented Aug 26, 2020

bkchr commented Aug 26, 2020

cecton commented Aug 30, 2020

cecton commented Sep 2, 2020

cecton commented Sep 9, 2020

ghost commented Sep 9, 2020

cecton commented Aug 21, 2020 •

edited

Loading

arkpar commented Aug 25, 2020 •

edited

Loading

bkchr commented Aug 25, 2020 •

edited by arkpar

Loading