From 06849bd0dab63596281d4241e76bfb9ac9e10a4e Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Fri, 10 Nov 2023 16:20:33 -0800 Subject: [PATCH 1/2] Addressed comments and added more tests --- .../parachains/src/assigner_bulk/mod.rs | 12 +- .../parachains/src/assigner_bulk/tests.rs | 147 +++++++++++++++++- polkadot/runtime/parachains/src/mock.rs | 23 +-- 3 files changed, 157 insertions(+), 25 deletions(-) diff --git a/polkadot/runtime/parachains/src/assigner_bulk/mod.rs b/polkadot/runtime/parachains/src/assigner_bulk/mod.rs index 137a5c3c6b65..d294e5fa9695 100644 --- a/polkadot/runtime/parachains/src/assigner_bulk/mod.rs +++ b/polkadot/runtime/parachains/src/assigner_bulk/mod.rs @@ -473,16 +473,14 @@ impl Pallet { } // Tests/Invariant: -// - After `assign_core`, WorkState is `Some`. // - next_schedule always points to next item in CoreSchedules. (handled by // next_schedule_always_points_to_next_work_plan_item) // - Test insertion in the middle, beginning and end: Should fail in all cases but the last. -// - Test insertion on empty queue. -// - Test overwrite vs insert: Overwrite no longer allowed - should fail with error. +// (handled by assign_core_enforces_higher_block_number) +// - Test insertion on empty queue. (handled by assign_core_works_with_no_prior_schedule) // - New: Test that assignments are served correctly. E.g. two equal assignments will be served as -// ABABAB ... and more importantly have a test that checks that core is shared fairly, even in -// case of `ratio` not being divisible by `step` (over multiple rounds). (handled by -// assign_core_enforces_higher_block_number) -// - Test insertion on empty queue. (Handled by assign_core_works_with_no_prior_schedule) +// ABABAB (handled by equal_assignments_served_equally) +// - Have a test that checks that core is shared fairly, even in case of `ratio` not being divisible +// by `step` (over multiple rounds). (handled by assignment_proportions_indivisible_by_step_work) // - Test overwrite vs insert: Overwrite no longer allowed - should fail with error. (handled using // Error::DuplicateInsert, though earlier errors should prevent this error from ever triggering) diff --git a/polkadot/runtime/parachains/src/assigner_bulk/tests.rs b/polkadot/runtime/parachains/src/assigner_bulk/tests.rs index 41a10fbfd6f3..0d1706588249 100644 --- a/polkadot/runtime/parachains/src/assigner_bulk/tests.rs +++ b/polkadot/runtime/parachains/src/assigner_bulk/tests.rs @@ -357,16 +357,16 @@ fn ensure_workload_works() { remaining: PartsOf57600::from(57600u16), }; - let expected_descriptor_1: CoreDescriptor> = + let empty_descriptor: CoreDescriptor> = CoreDescriptor { queue: None, current_work: None }; - let expected_descriptor_2 = CoreDescriptor { + let assignments_queued_descriptor = CoreDescriptor { queue: Some(QueueDescriptor { first: BlockNumberFor::::from(11u32), last: BlockNumberFor::::from(11u32), }), current_work: None, }; - let expected_descriptor_3 = CoreDescriptor { + let assignments_active_descriptor = CoreDescriptor { queue: None, current_work: Some(WorkState { assignments: vec![(CoreAssignment::Pool, test_assignment_state)], @@ -375,7 +375,6 @@ fn ensure_workload_works() { step: PartsOf57600::from(57600u16), }), }; - let expected_descriptor_4 = expected_descriptor_1.clone(); new_test_ext(GenesisConfigBuilder::default().build()).execute_with(|| { let mut core_descriptor: CoreDescriptor> = @@ -384,7 +383,7 @@ fn ensure_workload_works() { // Case 1: No new schedule in CoreSchedules for core BulkAssigner::ensure_workload(10u32, core_idx, &mut core_descriptor); - assert_eq!(core_descriptor, expected_descriptor_1); + assert_eq!(core_descriptor, empty_descriptor); // Case 2: New schedule exists in CoreSchedules for core, but new // schedule start is not yet reached. @@ -400,18 +399,18 @@ fn ensure_workload_works() { core_descriptor = CoreDescriptors::::get(core_idx); BulkAssigner::ensure_workload(10u32, core_idx, &mut core_descriptor); - assert_eq!(core_descriptor, expected_descriptor_2); + assert_eq!(core_descriptor, assignments_queued_descriptor); // Case 3: Next schedule exists in CoreSchedules for core. Next starting // block has been reached. Swaps new WorkState into CoreDescriptors from // CoreSchedules. BulkAssigner::ensure_workload(11u32, core_idx, &mut core_descriptor); - assert_eq!(core_descriptor, expected_descriptor_3); + assert_eq!(core_descriptor, assignments_active_descriptor); // Case 4: end_hint reached but new schedule start not yet reached. WorkState in // CoreDescriptor is cleared BulkAssigner::ensure_workload(15u32, core_idx, &mut core_descriptor); - assert_eq!(core_descriptor, expected_descriptor_4); + assert_eq!(core_descriptor, empty_descriptor); }); } @@ -551,5 +550,137 @@ fn assignment_proportions_in_core_state_work() { Some(PartsOf57600::from(57600u16 / 3 * 2)) ); } + + // Final check, task 2's turn to be served + assert_eq!( + BulkAssigner::pop_assignment_for_core(core_idx), + Some(BulkAssignment::Bulk(task_2.into())) + ); + }); +} + +#[test] +fn equal_assignments_served_equally() { + let core_idx = CoreIndex(0); + let task_1 = TaskId::from(1u32); + let task_2 = TaskId::from(2u32); + + new_test_ext(GenesisConfigBuilder::default().build()).execute_with(|| { + run_to_block(10, |n| if n == 10 { Some(Default::default()) } else { None }); + + // Tasks 1 and 2 get equal work parts + let test_assignments = vec![ + (CoreAssignment::Task(task_1), PartsOf57600::from(57600u16 / 2)), + (CoreAssignment::Task(task_2), PartsOf57600::from(57600u16 / 2)), + ]; + + assert_ok!(BulkAssigner::assign_core( + core_idx, + BlockNumberFor::::from(10u32), + test_assignments, + None, + )); + + // Test that popped assignments alternate between tasks 1 and 2 + assert_eq!( + BulkAssigner::pop_assignment_for_core(core_idx), + Some(BulkAssignment::Bulk(task_1.into())) + ); + + assert_eq!( + BulkAssigner::pop_assignment_for_core(core_idx), + Some(BulkAssignment::Bulk(task_2.into())) + ); + + assert_eq!( + BulkAssigner::pop_assignment_for_core(core_idx), + Some(BulkAssignment::Bulk(task_1.into())) + ); + + assert_eq!( + BulkAssigner::pop_assignment_for_core(core_idx), + Some(BulkAssignment::Bulk(task_2.into())) + ); + + assert_eq!( + BulkAssigner::pop_assignment_for_core(core_idx), + Some(BulkAssignment::Bulk(task_1.into())) + ); + + assert_eq!( + BulkAssigner::pop_assignment_for_core(core_idx), + Some(BulkAssignment::Bulk(task_2.into())) + ); + }); +} + +#[test] +// Checks that core is shared fairly, even in case of `ratio` not being +// divisible by `step` (over multiple rounds). +fn assignment_proportions_indivisible_by_step_work() { + let core_idx = CoreIndex(0); + let task_1 = TaskId::from(1u32); + let ratio_1 = PartsOf57600::from(57600u16 / 5 * 3); + let ratio_2 = PartsOf57600::from(57600u16 / 5 * 2); + let task_2 = TaskId::from(2u32); + + new_test_ext(GenesisConfigBuilder::default().build()).execute_with(|| { + run_to_block(10, |n| if n == 10 { Some(Default::default()) } else { None }); + + // Task 1 gets 3/5 core usage, while task 2 gets 2/5. That way + // step is set to 2/5 and task 1 is indivisible by step. + let test_assignments = + vec![(CoreAssignment::Task(task_1), ratio_1), (CoreAssignment::Task(task_2), ratio_2)]; + + assert_ok!(BulkAssigner::assign_core( + core_idx, + BlockNumberFor::::from(10u32), + test_assignments, + None, + )); + + // Pop 5 assignments. Should Result in the the following work ordering: + // 1, 2, 1, 1, 2. The remaining parts for each assignment should be same + // at the end as in the beginning. + assert_eq!( + BulkAssigner::pop_assignment_for_core(core_idx), + Some(BulkAssignment::Bulk(task_1.into())) + ); + + assert_eq!( + BulkAssigner::pop_assignment_for_core(core_idx), + Some(BulkAssignment::Bulk(task_2.into())) + ); + + assert_eq!( + BulkAssigner::pop_assignment_for_core(core_idx), + Some(BulkAssignment::Bulk(task_1.into())) + ); + + assert_eq!( + BulkAssigner::pop_assignment_for_core(core_idx), + Some(BulkAssignment::Bulk(task_1.into())) + ); + + assert_eq!( + BulkAssigner::pop_assignment_for_core(core_idx), + Some(BulkAssignment::Bulk(task_2.into())) + ); + + // Remaining should equal ratio for both assignments. + assert_eq!( + CoreDescriptors::::get(core_idx) + .current_work + .as_ref() + .and_then(|w| Some(w.assignments[0].1.remaining)), + Some(ratio_1) + ); + assert_eq!( + CoreDescriptors::::get(core_idx) + .current_work + .as_ref() + .and_then(|w| Some(w.assignments[1].1.remaining)), + Some(ratio_2) + ); }); } diff --git a/polkadot/runtime/parachains/src/mock.rs b/polkadot/runtime/parachains/src/mock.rs index 20bd55865e8e..ee3600bb1146 100644 --- a/polkadot/runtime/parachains/src/mock.rs +++ b/polkadot/runtime/parachains/src/mock.rs @@ -24,7 +24,8 @@ use crate::{ paras::ParaKind, paras_inherent, scheduler, scheduler::common::{ - AssignmentProvider, AssignmentProviderConfig, AssignmentVersion, V0Assignment, + AssignmentProvider, AssignmentProviderConfig, AssignmentVersion, FixedAssignmentProvider, + V0Assignment, }, session_info, shared, ParaId, }; @@ -464,15 +465,6 @@ pub mod mock_assigner { old } - // Provides a core count for scheduler tests defaulting to the most common number, - // 5, if no explicit count was set. - fn session_core_count() -> u32 { - match MockCoreCount::::get() { - Some(count) => count, - None => 5, - } - } - // With regards to popping_assignments, the scheduler just needs to be tested under // the following two conditions: // 1. An assignment is provided @@ -505,6 +497,17 @@ pub mod mock_assigner { } } } + + // Provides a core count for scheduler tests defaulting to the most common number, + // 5, if no explicit count was set. + impl FixedAssignmentProvider for Pallet { + fn session_core_count() -> u32 { + match MockCoreCount::::get() { + Some(count) => count, + None => 5, + } + } + } } impl mock_assigner::pallet::Config for Test {} From 15470015fa04832d0fdba820b10eb927011db966 Mon Sep 17 00:00:00 2001 From: BradleyOlson64 Date: Fri, 10 Nov 2023 16:23:45 -0800 Subject: [PATCH 2/2] Comment edit --- polkadot/runtime/parachains/src/assigner_bulk/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/polkadot/runtime/parachains/src/assigner_bulk/mod.rs b/polkadot/runtime/parachains/src/assigner_bulk/mod.rs index d294e5fa9695..30bddc3046dd 100644 --- a/polkadot/runtime/parachains/src/assigner_bulk/mod.rs +++ b/polkadot/runtime/parachains/src/assigner_bulk/mod.rs @@ -478,8 +478,8 @@ impl Pallet { // - Test insertion in the middle, beginning and end: Should fail in all cases but the last. // (handled by assign_core_enforces_higher_block_number) // - Test insertion on empty queue. (handled by assign_core_works_with_no_prior_schedule) -// - New: Test that assignments are served correctly. E.g. two equal assignments will be served as -// ABABAB (handled by equal_assignments_served_equally) +// - Test that assignments are served correctly. E.g. two equal assignments will be served as ABABAB +// (handled by equal_assignments_served_equally) // - Have a test that checks that core is shared fairly, even in case of `ratio` not being divisible // by `step` (over multiple rounds). (handled by assignment_proportions_indivisible_by_step_work) // - Test overwrite vs insert: Overwrite no longer allowed - should fail with error. (handled using