Adding support for Accelerators to GCE clusters. #45130
Conversation
vishh
commented
Apr 28, 2017
vishh
commented
Signed-off-by: Vishnu kannan <[email protected]>
k8s-ci-robot
added
the
cncf-cla: yes
|
This change is |
k8s-github-robot
added
approved
|
@dashpole will you be able to review this PR? |
| local attempt=1 | ||
| while true; do | ||
| echo "Attempt ${attempt} to create ${1}" >&2 | ||
| if ! ${gcloud} compute instance-templates create \ | ||
| if ! ${gcloud} beta compute instance-templates create \ |
There was a problem hiding this comment.
@mikedanese @zmerlynn any objections towards switching OSS to using beta APIs?
There was a problem hiding this comment.
should we do similar to this PR in gke? cc @roberthbailey
There was a problem hiding this comment.
I have a change list out for GKE already.
There was a problem hiding this comment.
@mikedanese can I assume no objections towards this change then?
| @@ -55,6 +58,11 @@ if [[ "${NODE_OS_DISTRIBUTION}" == "cos" ]]; then | |||
| NODE_OS_DISTRIBUTION="gci" | |||
| fi | |||
|
|
|||
| # GPUs supported in GCE do not have compatible drivers in Debian 7. | |||
| if [[ "${NODE_OS_DISTRIBUTION}" == "debian" ]]; then | |||
There was a problem hiding this comment.
We are blacklisting debian for GPU support. Should we instead whitelist gci? May be better if we want to add more OS distributions (ubuntu?) in the future.
There was a problem hiding this comment.
ubuntu should work as well. Hence the blacklist.
There was a problem hiding this comment.
Ok, in that case this is a good approach.
| # VMs with Accelerators cannot be live migrated. | ||
| # More details here - https://cloud.google.com/compute/docs/gpus/add-gpus#create-new-gpu-instance | ||
| if [[ ! -z "${NODE_ACCELERATORS}" ]]; then | ||
| accelerator_args="--maintenance-policy TERMINATE --restart-on-failure --accelerator ${NODE_ACCELERATORS}" |
There was a problem hiding this comment.
does it matter that terminate is all caps? Looks like the documentation has it lowercase...
There was a problem hiding this comment.
jk, found the full example here
|
code changes lgtm. feel free to add the label once you figure out beta apis question. |
|
/approve |
|
adding lgtm tag based on #45130 (comment) /lgtm |
vishh
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mikedanese, vishh
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
Automatic merge from submit-queue (batch tested with PRs 44830, 45130) |
