Added the option to specify a machine_type in software for gcloud build #229
Conversation
…machine_type param when triggering google cloud build
…mment for the enum type in the server code
| "--machine_type", | ||
| default="", | ||
| help="Optional machine type for Google Cloud Build to use for building this software.", | ||
| type=click.Choice(["standard", "n1-highcpu-8", "n1-highcpu-32", ''], case_sensitive=False) |
There was a problem hiding this comment.
What machine is "standard"?
What's the deal with the e2 machines?
https://cloud.google.com/build/pricing
There was a problem hiding this comment.
The default machine is "e2-medium". Maybe I should refer to it as "default" instead of "standard"?
Also, it turns out I was using an old version of the gcloud docker, so that's why I couldn't get the e2 machines to work. I'm working on a commit that'll add those.
It might be good to have some indication that the default is e2-medium, but you can't actually choose e2-medium from the command line, so allowing the user to choose e2-medium in carrot would just translate to whatever the default is in google cloud, which might change. Thoughts?
| gcloud builds submit --tag ~{registry_host}/~{software_name}:~{commit_hash} --timeout=24h | ||
| if [ -z ~{machine_type} ] | ||
| then | ||
| gcloud builds submit --tag ~{registry_host}/~{software_name}:~{commit_hash} --timeout=24h |
There was a problem hiding this comment.
Instead of adding this switch here, why not add a default option?
There was a problem hiding this comment.
A default option for machine_type? It seems like you can only get the default machine by not using the --machine-type flag, so I think I need the switch.
| /// | ||
| /// Google's Cloud Build docs explain the machine-type argument here: | ||
| /// https://cloud.google.com/sdk/gcloud/reference/builds/submit#--machine-type | ||
| /// Notably, it mentions two other machine types: e2-highcpu-8 and e2-highcpu-32, but, when I |
| #[derive(Debug, PartialEq, DbEnum, Serialize, Deserialize, Clone, Copy)] | ||
| #[DieselType = "Machine_type_enum"] | ||
| pub enum MachineTypeEnum { | ||
| #[serde(rename = "n1-highcpu-8")] |
There was a problem hiding this comment.
So does this store the value in the DB with - instead of as an identifier? Why is that desireable/necessary?
There was a problem hiding this comment.
The version with dashes is the format google cloud build actually uses, so I wanted what's in the database to match that, so using the value from the database is easier and users see the actual value that is submitted.
| @@ -12,7 +12,7 @@ use crate::models::software_version::{ | |||
| use crate::requests::cromwell_requests::{CromwellClient, CromwellRequestError}; | |||
| use crate::util::temp_storage; | |||
| use diesel::PgConnection; | |||
| use serde_json::json; | |||
| use serde_json::{json, Value}; | |||
| "--machine_type", | ||
| default="", | ||
| help="Optional machine type for Google Cloud Build to use for building this software.", | ||
| type=click.Choice(["standard", "n1-highcpu-8", "n1-highcpu-32", ''], case_sensitive=False) |
There was a problem hiding this comment.
I would probably add the actual machine type for standard as well if that's possible so people can choose between "Give me the default" and "Give me this specific small machine" in case the default changes in the future.
There was a problem hiding this comment.
That makes sense. I can make that change
There was a problem hiding this comment.
Turns out you cannot actually supply the default machine type to the gcloud builds --machine-type build, so I'm not sure if letting the user choose it here would actually be meaningful?
There was a problem hiding this comment.
bizarre, lets leave it as is then
|
@lbergelson @jamesemery @droazen I'm curious what y'all have for opinions on this, if any: I limited the values for machine-type to only the values I've actually gotten to work: There is also the option in google cloud build to set up a private pool of machines (maybe not a bad option for us if we mainly have need of high memory machines, not high cpu) and that provides a longer list of options. Should I modify this to allow specifying all of the options in the full private pool list? Maybe then I can include messages in the CLI or somewhere to indicate the differences in requirements for the different options? It maybe should also be noted that this was originally intended as a quick fix for carrot tests failing in gatk because the builds were using too much memory. |
|
@KevinCLydon I'm in favor of allowing the additional options, or even any potential input to future proof. I think it also makes sense to add a warning message when using ones you can't vouch for. Since this is meant to be a quick fix I don't think either change is necessary at this time but might be worth doing in the future. |
…ker build wdl to support the up-to-date list of machine types
Google Cloud Build allows specifying a machine type to use for running the build. It has become clear that it is necessary to allow the user to specify this for their software builds because the recent gatk builds have failed because of OOM errors.
This is implemented by added a
machine_typecolumn tosoftware, which corresponds to the different options for machine types allowed by google cloud build. If a value other thanstandardis specified when creating/updating a software, builds for that software will use the specified machine type.