[pallet-broker] add extrinsic to remove a lease #7026
Conversation
Co-authored-by: Bastian Köcher <[email protected]>
There was a problem hiding this comment.
Functionality wise, this looks good for this extrinsic.
If somebody wants to do more, for example remove a lease AND remove the associated task from its core in the next sale, they can pair remove_lease with remove_from_workplan (or whatever that extrinsic will be called).
|
bot bench substrate-pallet --pallet=pallet_broker |
|
@ethernomad Requester could not be detected as a member of an allowed organization. |
|
Can someone run the bench bot for me? |
|
bot bench substrate-pallet --pallet=pallet_broker |
|
@re-gius https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7966356 was started for your command Comment |
|
@re-gius https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7966357 was started for your command Comment |
|
@re-gius https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7966358 was started for your command Comment |
|
@re-gius Command |
…=coretime-rococo --runtime_dir=coretime --target_dir=cumulus --pallet=pallet_broker
|
@re-gius Command |
…=coretime-westend --runtime_dir=coretime --target_dir=cumulus --pallet=pallet_broker
|
@re-gius Command |
Co-authored-by: Dónal Murray <[email protected]>
Co-authored-by: Dónal Murray <[email protected]>
|
/cmd bench --runtime dev --pallet pallet_broker |
|
Command "bench --runtime dev --pallet pallet_broker" has started 🚀 See logs here |
| T::AdminOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; | ||
|
|
||
| #[extrinsic_call] | ||
| _(origin as T::RuntimeOrigin, task); |
There was a problem hiding this comment.
the worst case would be removing the last task in the vec. Here it removes the first one.
Though in practice the cost should be negligible as MaxLeasedCores isn't so big.
There was a problem hiding this comment.
Agreed, would be nice to target the worst case, especially here where it's as straightforward as just changing the task id.
|
Command "bench --runtime dev --pallet pallet_broker" has finished ✅ See logs here Subweight results:
Command output:✅ Successful benchmarks of runtimes/pallets: |
| /// Remove a lease. | ||
| /// | ||
| /// - `origin`: Must be Root or pass `AdminOrigin`. | ||
| /// - `task`: The workload of the lease which should be removed. |
There was a problem hiding this comment.
| /// - `task`: The workload of the lease which should be removed. | |
| /// - `task`: The task id of the lease which should be removed. |
Also from memory there can be multiple leases for a given task_id, although not how this is intended to be used in the system. I don't think it's worth complicating the extrinsic to also check timeslice etc. You should however include in the docs that it removes the first in the vector, which is not necessarily the first to end in time. I'm AFK so can't check right now, but would be good to also cover this behaviour in the test.
| T::AdminOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; | ||
|
|
||
| #[extrinsic_call] | ||
| _(origin as T::RuntimeOrigin, task); |
There was a problem hiding this comment.
Agreed, would be nice to target the worst case, especially here where it's as straightforward as just changing the task id.
Description
#6929 requests more extrinsics for "managing the network's coretime allocations without needing to dabble with migration+runtime upgrade or set/kill storage patterns"
This pull request implements the remove_lease() extrinsic.
Integration
Downstream projects need to benchmark the weight for the remove_lease() extrinsic.
Review Notes
Mentorship is requested to ensure this is implemented correctly.
The lease is removed from state using the TaskId as a key. Is this sufficient. Does the extrinsic need to do anything else?