-
Notifications
You must be signed in to change notification settings - Fork 98
feat: Add an optional argument to set memory allocation #596
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
7d4ca3a
2cac422
cac5ed2
5e232c6
8a85987
7b4027a
f15caf1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,9 +5,11 @@ use crate::lib::error::{DfxError, DfxResult}; | |
| use crate::lib::message::UserMessage; | ||
|
|
||
| use clap::{App, Arg, ArgMatches, SubCommand}; | ||
| use ic_http_agent::{Agent, Blob, CanisterAttributes, ComputeAllocation, RequestId}; | ||
| use ic_http_agent::{ | ||
| Agent, Blob, CanisterAttributes, ComputeAllocation, MemoryAllocation, RequestId, | ||
| }; | ||
| use slog::info; | ||
| use std::convert::TryInto; | ||
| use std::convert::{TryFrom, TryInto}; | ||
| use tokio::runtime::Runtime; | ||
|
|
||
| pub fn construct() -> App<'static, 'static> { | ||
|
|
@@ -42,13 +44,23 @@ pub fn construct() -> App<'static, 'static> { | |
| .default_value("0") | ||
| .validator(compute_allocation_validator), | ||
| ) | ||
| .arg( | ||
| Arg::with_name("memory-allocation") | ||
| .help(UserMessage::InstallMemoryAllocation.to_str()) | ||
| .long("memory-allocation") | ||
| .short("m") | ||
| .takes_value(true) | ||
| .default_value("8GB") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you want to duplicate the information about the default here? I think the two sensible design spaces are:
With this code, there are now two components both defining a default value; that’s odd.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fine with the first approach. Btw, the same happens for compute allocation and I don't remember why it was done like that.
No, there's two components that follow the default value set by a common reference (the public spec) :) But yeah it's probably redundant since the replica will enforce the default anyway.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wouldn’t consider it “following the public spec” if the client makes an optional field mandatory and hides the default value (which may change in later versions). But this is getting philosophical and we are in agreement :-) Maybe the public spec shouldn’t be the place to define default values… oh well.
Yes, we should do the same for that too.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This would mean that a concrete Internet Computer implementation could choose an arbitrary (within the specified range) default value if none is specified, which would mean that if I use Anyway, I already fell in your trap
and as I said I'm fine with your first suggestion.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd much prefer the first approach.
I think the mistake is treating it as a "default value" at the entry point. The spec just says if the value isn't specified the system should treat it like 2^33. That's at the instanciation point. Technically we could not have a field at all and still be spec compliant. The interface definition is clear that the field is optional, so there should be nothing on the wire.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Glad to see everyone in agreement :-) |
||
| .validator(memory_allocation_validator), | ||
| ) | ||
| } | ||
|
|
||
| async fn install_canister( | ||
| env: &dyn Environment, | ||
| agent: &Agent, | ||
| canister_info: &CanisterInfo, | ||
| compute_allocation: ComputeAllocation, | ||
| memory_allocation: MemoryAllocation, | ||
| ) -> DfxResult<RequestId> { | ||
| let log = env.get_logger(); | ||
| let canister_id = canister_info.get_canister_id().ok_or_else(|| { | ||
|
|
@@ -70,7 +82,10 @@ async fn install_canister( | |
| &canister_id, | ||
| &Blob::from(wasm), | ||
| &Blob::empty(), | ||
| &CanisterAttributes { compute_allocation }, | ||
| &CanisterAttributes { | ||
| compute_allocation, | ||
| memory_allocation, | ||
| }, | ||
| ) | ||
| .await | ||
| .map_err(DfxError::from) | ||
|
|
@@ -85,6 +100,35 @@ fn compute_allocation_validator(compute_allocation: String) -> Result<(), String | |
| Err("Must be a percent between 0 and 100".to_string()) | ||
| } | ||
|
|
||
| fn parse_memory_allocation(memory_allocation: String) -> Result<u64, String> { | ||
|
dsarlis marked this conversation as resolved.
Outdated
|
||
| let split_point = memory_allocation.find(|c: char| !c.is_numeric()); | ||
| let memory_allocation = memory_allocation.trim(); | ||
| let (raw_num, unit) = split_point.map_or_else( | ||
| || (memory_allocation, ""), | ||
| |p| memory_allocation.split_at(p), | ||
| ); | ||
| let raw_num = raw_num | ||
| .parse::<u64>() | ||
| .map_err(|_| format!("Could not parse number: {}", raw_num))?; | ||
| let unit = unit.trim(); | ||
| match unit { | ||
| "KB" => Ok(raw_num * 1024), | ||
| "MB" => Ok(raw_num * 1024 * 1024), | ||
| "GB" => Ok(raw_num * 1024 * 1024 * 1024), | ||
| _ => return Err(format!("Invalid unit for memory allocation, {}. Expected one of <KB|MB|GB>", unit)), | ||
| } | ||
| } | ||
|
|
||
| fn memory_allocation_validator(memory_allocation: String) -> Result<(), String> { | ||
| let num = parse_memory_allocation(memory_allocation)?; | ||
|
|
||
| if num <= (1 << 48) { | ||
| Ok(()) | ||
| } else { | ||
| Err("Must be a number of bytes between 0 and 2^48".to_string()) | ||
| } | ||
| } | ||
|
|
||
| pub fn exec(env: &dyn Environment, args: &ArgMatches<'_>) -> DfxResult { | ||
| let log = env.get_logger(); | ||
| let config = env | ||
|
|
@@ -101,6 +145,16 @@ pub fn exec(env: &dyn Environment, args: &ArgMatches<'_>) -> DfxResult { | |
| .try_into() | ||
| .expect("Compute Allocation must be a percentage."); | ||
|
|
||
| let memory_allocation = MemoryAllocation::try_from( | ||
| parse_memory_allocation( | ||
| args.value_of("memory-allocation") | ||
| .unwrap_or("8GB") | ||
| .to_string(), | ||
| ) | ||
| .unwrap(), | ||
| ) | ||
| .expect("Memory Allocation must be a number between 0 and 2^48"); | ||
|
|
||
| let mut runtime = Runtime::new().expect("Unable to create a runtime"); | ||
|
|
||
| if let Some(canister_name) = args.value_of("canister_name") { | ||
|
|
@@ -110,6 +164,7 @@ pub fn exec(env: &dyn Environment, args: &ArgMatches<'_>) -> DfxResult { | |
| &agent, | ||
| &canister_info, | ||
| compute_allocation, | ||
| memory_allocation, | ||
| ))?; | ||
|
|
||
| if args.is_present("async") { | ||
|
|
@@ -132,6 +187,7 @@ pub fn exec(env: &dyn Environment, args: &ArgMatches<'_>) -> DfxResult { | |
| &agent, | ||
| &canister_info, | ||
| compute_allocation, | ||
| memory_allocation, | ||
| ))?; | ||
|
|
||
| if args.is_present("async") { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this only for testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, yes and no :)
I needed to point to a commit past
v0.5.5indfinityto make things work for my demo last week. I'll use what I'm told here (presumablyv0.5.6). Better yet, if someone bumpsdfinityseparately (say for a new SDK release) until this is merged and it includes the commit I'm interested in, I don't need to touch this at all.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, muting the thread without approving. Just ping us if you do need a change that needs an infra review!