Scheduler Module #1162

rphmeier · 2020-05-28T02:28:16Z

This implements the Scheduler module from the implementor's guide. It makes some small adjustments to the guide as well, as certain invariants needed cleaning up.

The implementation itself is done, but I still need to add a few tests. In the meantime review on the main body of code can already begin.

runtime/parachains/src/scheduler.rs

shawntabrizi · 2020-06-07T15:51:06Z

runtime/parachains/src/scheduler.rs

+decl_storage! {
+	trait Store for Module<T: Trait> as ParaScheduler {
+		/// All the validator groups. One for each core.
+		ValidatorGroups: Vec<Vec<ValidatorIndex>>;


Could you annotate length bounds for the vectors used in storage?

"one for each core" is already kind of a length bound

Added some more concrete length bounds. follow-up PR welcome to re-format or rephrase

drahnr

A few minor nitpicks, a more in detail review than a birds-eye-review. Comparing to the guide was done partially.

drahnr · 2020-06-08T09:58:29Z

roadmap/implementors-guide/guide.md

@@ -605,25 +609,37 @@ struct ParathreadEntry {
 // A queued parathread entry, pre-assigned to a core.


Use doc comments /// instead of //

roadmap/implementors-guide/guide.md

runtime/parachains/src/scheduler.rs

drahnr · 2020-06-08T10:10:50Z

runtime/parachains/src/scheduler.rs

+//!   manipulate the scheduler to assign themselves as desired.
+//! - High or close to optimal throughput of parachains and parathreads. Work among validator groups should be balanced.
+//!
+//! The Scheduler manages resource allocation using the concept of "Availability Cores".


Can we document the concept a bit more or directly reference what this means, if we introduce this name, state that. It's not search-engine-friendly.

Agree that it would be useful if this were documented in some more detail here. If not that, then at least mentioning that this is covered in more detail in the guide would help. A new programmer coming into this code base won't necessarily even know that the implementer's guide exists. They'll look at the code and at the inline documentation instead.

The guide has a lot of detail on this concept, but PRs welcome to improve it!

runtime/parachains/src/scheduler.rs

roadmap/implementors-guide/guide.md

coriolinus · 2020-06-08T11:36:19Z

roadmap/implementors-guide/guide.md

+  - Behavior undefined if any given cores were not scheduled.
+  - Behavior undefined if the given cores are not sorted ascending by core index


I get that we may not wish to pay for runtime checks of these properties, but it feels like a warning flag when we introduce the possibility of UB; those tend to be the difficult bugs to discover, when they occur. Maybe we could introduce a compilation feature like ub-checks which does do the appropriate runtime checks?

It depends what the runtime checks are.

When within the runtime, panicking is pretty much the worst thing you can do. Contrary to typical software, you don't want to fail fast and fail hard - you want to limp along in broken state and get fixed by governance.

Checks that print to console and return as no-op would be welcome.

I don't think we'd ever run with ub-checks enabled on a "real" node. Instead, they'd be a debugging tool, maybe in CI, just to ensure that the properties we expect haven't been violated.

Agreed, although I don't perceive this as a blocker for the PR, better as a follow-on that would make a good first issue for someone

coriolinus · 2020-06-08T12:03:50Z

runtime/parachains/src/initializer.rs

+			let random_hash = T::Randomness::random(&b"paras"[..]);
+			let len = sp_std::cmp::min(32, random_hash.as_ref().len());
+			buf[..len].copy_from_slice(&random_hash.as_ref()[..len]);
+			buf


Do we care that buf always has 32-len trailing 0s?

Nope! In practice hash is always 32 bytes, and 0-padding it is pretty much the best option if the hash is smaller in some case. It doesn't lose us any security in any case.

In the case that the hash type is larger (again, unlikely), 32 random bytes are more than enough for our purposes.

runtime/parachains/src/paras.rs

coriolinus · 2020-06-08T12:25:39Z

runtime/parachains/src/scheduler.rs

+//!   manipulate the scheduler to assign themselves as desired.
+//! - High or close to optimal throughput of parachains and parathreads. Work among validator groups should be balanced.
+//!
+//! The Scheduler manages resource allocation using the concept of "Availability Cores".


Agree that it would be useful if this were documented in some more detail here. If not that, then at least mentioning that this is covered in more detail in the guide would help. A new programmer coming into this code base won't necessarily even know that the implementer's guide exists. They'll look at the code and at the inline documentation instead.

coriolinus · 2020-06-08T12:28:36Z

roadmap/implementors-guide/guide.md

@@ -605,25 +609,37 @@ struct ParathreadEntry {
 // A queued parathread entry, pre-assigned to a core.
 struct QueuedParathread {
 	claim: ParathreadEntry,
-	core: CoreIndex,
+	// this value is between 0 and config.parathread_cores.
+	core_offset: u32,


In several places in this document, you remove a CoreIndex and insert something named *_offset: u32. However, down in scheduler.rs, you define a CoreIndex type anyway. This leaves me confused: is the type still necessary, just not in these places, or have the documentation and implementation gotten out of sync?

These are offsets, not core indices, so it would break an invariant that CoreIndex always refers to an actual core index, if that makes sense. I believe the implementation and guide are coherent on this.

coriolinus · 2020-06-08T12:54:44Z

runtime/parachains/src/scheduler.rs

+	// Queue a parathread entry to be processed.
+	//
+	// Provide the entry and the number of parathread cores, which must be greater than 0.
+	fn queue_entry(&mut self, entry: ParathreadEntry, n_parathread_cores: u32) {


Suggested change

fn queue_entry(&mut self, entry: ParathreadEntry, n_parathread_cores: u32) {

fn enqueue_entry(&mut self, entry: ParathreadEntry, n_parathread_cores: u32) {

With the current naming, it looks more like the Entry API than an insertion.

Documentation suggests that n_parathread_cores should always be equal to config.parathread_cores. Is it plausible to fetch that value from configuration within this function instead of passing it as an argument?

It would be possible to fetch from storage, but IMO it's not better for a few reasons:

We'd have to attach the T: Trait to this type, which would be weird

We'd have to decode the entire configuration for every enqueued parathread claim

runtime/parachains/src/scheduler.rs

Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com> Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>

rphmeier added the A3-in_progress Pull request is in progress. No review needed at this stage. label May 28, 2020

rphmeier force-pushed the rh-para-scheduler branch 2 times, most recently from f771d9a to f0ea8a1 Compare May 30, 2020 04:31

rphmeier added the B0-silent Changes should not be mentioned in any release notes label Jun 1, 2020

rphmeier force-pushed the rh-para-scheduler branch from f0ea8a1 to 5e88e30 Compare June 1, 2020 21:12

paritytech-ci added the B2-breaksapi label Jun 1, 2020

scheduler module skeleton

af77a9a

rphmeier force-pushed the rh-para-scheduler branch from 5e88e30 to af77a9a Compare June 2, 2020 16:44

rphmeier added 13 commits June 2, 2020 18:57

update scheduler skeleton to match latest version of guide

d2eb247

better session change notification

631a6ba

add mock randomness and fix test compilation

cdf114b

shuffle validators into groups

b2a8c42

finish implementing session change logic for scheduler

d6d751d

tweak core assignment type to track retries of parathread

ba8dc9f

reframe queued parathread core as offset

be800dd

implement initialzation and finalization routines

cad53e6

implement parathread claim queuing

eb64044

implement core_para

8b4cfd4

implement the group_validators routine and fix errors

ff2e4d7

add a reason for freeing cores

35a3692

implement schedule function

adbaa1d

bkchr reviewed Jun 3, 2020

View reviewed changes

runtime/parachains/src/scheduler.rs Outdated Show resolved Hide resolved

rphmeier added 8 commits June 6, 2020 12:39

Merge branch 'master' into rh-para-scheduler

e5c1651

add some docs to the scheduled function

6a11337

implement occupied helper

c63a94b

implement availability predicate

1c6d4ab

fix some warnings

83ba678

integrate scheduler into initializer

d9df378

integrate scheduler into mock module

3e7ac7b

avoid conflict with Substrate's scheduler storage

1bee255

rphmeier added 6 commits June 6, 2020 13:49

add parathreads index to paras module

9096c14

implement parathreads map in paras module

242b129

add is_parathread to paras

94cb169

test adding parathread claim

55ad79d

test that you cannot add claims when no parathread cores exist

2703687

check session change parathread queue pruning

f707371

rphmeier added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jun 6, 2020

rphmeier marked this pull request as ready for review June 6, 2020 19:13

rphmeier requested review from coriolinus and drahnr June 6, 2020 19:13

rphmeier added 4 commits June 6, 2020 15:32

test validator shuffling

40d39eb

add allow_unused to scheduler items

19f6d53

add test for scheduling

f5ef2fa

add some more tests for scheduling logic

2d52e05

shawntabrizi reviewed Jun 7, 2020

View reviewed changes

rphmeier added 3 commits June 7, 2020 17:25

test core rotation

56e8669

check parathread claim pruning after retries

a909ad8

add bound notes

89ad0f8

drahnr reviewed Jun 8, 2020

View reviewed changes

coriolinus reviewed Jun 8, 2020

View reviewed changes

rphmeier and others added 3 commits June 10, 2020 14:13

Apply suggestions from code review

f85ff4e

Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com> Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>

more suggestions from review

50965e8

test availability predicate, add box to please compiler

3918713

rphmeier mentioned this pull request Jun 11, 2020

Inclusion Module #1242

Merged

rphmeier added 2 commits June 11, 2020 12:54

Merge branch 'master' into rh-para-scheduler

3392398

add changes to guide

63dfb9c

rphmeier merged commit 1696982 into master Jun 11, 2020

rphmeier deleted the rh-para-scheduler branch June 11, 2020 19:15

rphmeier mentioned this pull request Sep 26, 2022

Member Request polkadot-fellows/seeding#4

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scheduler Module #1162

Scheduler Module #1162

rphmeier commented May 28, 2020 •

edited

Loading

shawntabrizi Jun 7, 2020 •

edited

Loading

rphmeier Jun 7, 2020

rphmeier Jun 7, 2020

drahnr left a comment

drahnr Jun 8, 2020

drahnr Jun 8, 2020

coriolinus Jun 8, 2020

rphmeier Jun 8, 2020

coriolinus Jun 8, 2020

rphmeier Jun 10, 2020

coriolinus Jun 11, 2020

rphmeier Jun 11, 2020 •

edited

Loading

coriolinus Jun 8, 2020

rphmeier Jun 10, 2020

coriolinus Jun 8, 2020

coriolinus Jun 8, 2020

rphmeier Jun 10, 2020

coriolinus Jun 8, 2020

coriolinus Jun 8, 2020

rphmeier Jun 10, 2020

		@@ -605,25 +609,37 @@ struct ParathreadEntry {
		// A queued parathread entry, pre-assigned to a core.

		- Behavior undefined if any given cores were not scheduled.
		- Behavior undefined if the given cores are not sorted ascending by core index

	fn queue_entry(&mut self, entry: ParathreadEntry, n_parathread_cores: u32) {
	fn enqueue_entry(&mut self, entry: ParathreadEntry, n_parathread_cores: u32) {

Scheduler Module #1162

Scheduler Module #1162

Conversation

rphmeier commented May 28, 2020 • edited Loading

shawntabrizi Jun 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drahnr left a 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

rphmeier Jun 11, 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

rphmeier commented May 28, 2020 •

edited

Loading

shawntabrizi Jun 7, 2020 •

edited

Loading

rphmeier Jun 11, 2020 •

edited

Loading