netgroup-polito / CrownLabs

Kubernetes-based Remote Laboratories
https://crownlabs.polito.it
Apache License 2.0
107 stars 42 forks source link

Removed quotas cap #875

Closed cheina97 closed 11 months ago

cheina97 commented 1 year ago

Description

This PR removes the cap on CPU and memory for tenants.

@QcFe can you confirm if it is secure? If I remember well a normal user cannot customize it's tenant.

kingmakerbot commented 1 year ago

Hi @cheina97. Thanks for your PR.

I am @kingmakerbot. You can interact with me issuing a slash command in the first line of a comment. Currently, I understand the following commands:

Make sure this PR appears in the CrownLabs changelog, adding one of the following labels:

QcFe commented 1 year ago

Thanks, I think it's considerable safe, however, I'd not take the feature away but possibly make it configurable through args & chart... (with a default of 0 meaning no limit)

frisso commented 1 year ago

Wouldn't be possible to set those quota parameters as properties of the tenant itself? With "zero" as "no limit"?

cheina97 commented 1 year ago

Wouldn't be possible to set those quota parameters as properties of the tenant itself? With "zero" as "no limit"?

They should be added in the tenant spec. This means a change in the Crownlabs API. I think it should be a good improvement, but it may take some time.

QcFe commented 1 year ago

Would you mind setting the new values inside the helm chart too? Setting as default the old values that were hardcoded...

cheina97 commented 1 year ago

/rebase

cheina97 commented 1 year ago

Would you mind setting the new values inside the helm chart too? Setting as default the old values that were hardcoded...

Of course 👍

cheina97 commented 11 months ago

@QcFe PR is ready or review

frisso commented 11 months ago

If we want to give resources to our students that exceed their capabilities at home, I feel that, per each instance, students should be allowed to create up to 4 cores and 16GB RAM at least (personally, I would say even 8 and 32). Max 10 instances, instead, looks fair to me. @QcFe @cheina97 what do you think?

cheina97 commented 11 months ago

If we want to give resources to our students that exceed their capabilities at home, I feel that, per each instance, students should be allowed to create up to 4 cores and 16GB RAM at least (personally, I would say even 8 and 32). Max 10 instances, instead, looks fair to me. @QcFe @cheina97 what do you think?

At the moment the caps are 25 for the CPU, 50GB for RAM and 10 for the instances. What do you think about putting a huge amount of resources as cap? Something like 150 CPU, 300GB of RAM, and 50 instances? Remember that this limit is for all tenants (not only students). The limits of student's resources should be managed with workspaces.

frisso commented 11 months ago

If we want to give resources to our students that exceed their capabilities at home, I feel that, per each instance, students should be allowed to create up to 4 cores and 16GB RAM at least (personally, I would say even 8 and 32). Max 10 instances, instead, looks fair to me. @QcFe @cheina97 what do you think?

At the moment the caps are 25 for the CPU, 50GB for RAM and 10 for the instances. What do you think about putting a huge amount of resources as cap? Something like 150 CPU, 300GB of RAM, and 50 instances? Remember that this limit is for all tenants (not only students). The limits of student's resources should be managed with workspaces.

Current limits (25 CPU, 50GB, 10 instances) look perfect to me.

frisso commented 11 months ago

LGTM, for me we can merge.

cheina97 commented 11 months ago

/merge

cheina97 commented 11 months ago

/rebase

cheina97 commented 11 months ago

/merge