acorn-io / runtime

A simple application deployment framework built on Kubernetes
https://docs.acorn.io/
Apache License 2.0
1.13k stars 100 forks source link

Add resources field to computeclass #2380 #2384

Closed dciangot closed 11 months ago

dciangot commented 11 months ago

The edited version of the computeclass will allow to specify any kind of resources allowed by K8s corev1.ResourceRequirements. In principle that should be enough to enable admins to create computeclass with GPU and accelarators in general.

NOTES/QUESTIONS

Checklist

cjellick commented 11 months ago

Context is this slack thread: https://acorn-users.slack.com/archives/C03R9ME0SKC/p1702567598185389

@ibuildthecloud effectively blessed the approach conceptually

dciangot commented 11 months ago

Thank you for your contribution!

Additional to my comments, I believe you also need to check in the changes generated from running make generate. The CI would have caught that had it run.

Thank you @thedadams , indeed I missed that make step, sorry about that.

Although there are two open comments, I pushed the changes that I found more in line with my understanding. Also, I added the update to the admin documentation, that I completely missed before.

dciangot commented 11 months ago

the recent commit broke the new tests for computeclass, I have rebased on the current main, hope it is ok for you

cjellick commented 11 months ago

Looks like a new commit came in. WIll need rereviewed @thedadams @tylerslaton

Tyler, once this is good, can you own getting it merged and shepherding it along internally (whatever that may mean...actually dont think theres much to do there other than merge)

dciangot commented 11 months ago

Yeah, it was on the latest @thedadams request. Now integration is failing, not related to the changes I made though. I can take a look later, meanwhile any hint is welcome.

cjellick commented 11 months ago

we'll rekick the tests after it officially fails to see if it was a flake

tylerslaton commented 11 months ago

Merging now, thanks for the contribution @dciangot!