Skip to content
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

remove requirement for CPU and memory #218

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

technosophos
Copy link
Contributor

This was pulled from #199 into its own issue so we could continue this dicsussion

Closes #182

Signed-off-by: Matt Butcher [email protected]

@technosophos
Copy link
Contributor Author

/cc @BharatNarasimman

| `cpu` | [`CPU`](#cpu) | Y | | Specifies the attributes of the cpu resource required for the container. |
| `memory` | [`Memory`](#memory) | Y | | Specifies the attributes of the memory resource required for the container. |
| `cpu` | [`CPU`](#cpu) | N | | Specifies the attributes of the cpu resource required for the container. |
| `memory` | [`Memory`](#memory) | N | | Specifies the attributes of the memory resource required for the container. |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that making platform assume defaults for CPU and Memory can lead to difficult to debug container activation issues for the application operator.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if we allow relying on platforms defaults, then we should add a way by which the operator can specify resources in the application configuration so that the component can be deployed to a platform whose defaults are too low for that component.


In reply to: 335190832 [](ancestors = 335190832)

@BharatNarasimman
Copy link
Contributor

| resources | Resources | Y | | Resources required by the container. |

if we want to make CPU and Memory as optional, then we should prbly make 'resources' attribute optional as well.


Refers to: 3.component_model.md:196 in 60f42d9. [](commit_id = 60f42d9, deletion_comment = False)

@vturecek
Copy link
Member

I think the premise of #182 is wrong. Resources is not optional on container.

CPU and memory are both currently required no matter what. The resources attribute on container is required, and the CPU and memory attributes on resources are required too. So users always have to provide CPU and memory requirements.

As @BharatNarasimman mentioned, we'd probably want to make the entire resources attribute optional if we're going to make all resource requirements therein optional. But if we decide to make any of them required in the future, we'd have to go back and make resources required again.

@technosophos
Copy link
Contributor Author

The problem is that devs have NO IDEA what to set those fields to. What percentage of a CPU does the helloworld-python take up? Do you have an educated estimate? Because I don't. This is what led to the people running out of space on the cluster because they set it to 1, which consumed an entire CPU on an 8-CPU cluster.

@technosophos technosophos added this to the backlog milestone Oct 15, 2019
@technosophos
Copy link
Contributor Author

I closed 182, and we can continue here the discussion on whether CPU and memory should be required

@hongchaodeng
Copy link
Member

Well, in our production experience, dev has no sense of setting the right amount of CPU/MEM. Most of the time the ops can override them.

It holds true that CPU and memory are both required no matter what. But what if it is set by the ops? I imagine it is predefined by an application profile (high SLO, low latency => 8 cores) or automatically set by historical estimates.

@technosophos
Copy link
Contributor Author

The point of the resources section was for devs to set a minimum requirement. So, for example, if using Scala, the devs would likely know that at minimum, they need 512M of memory. Ops can then override with a higher value.

But to make it required is probably a bad idea. I am concerned that devs will default to too-high values. Like asking for 1 CPU when they only need 0.2.

@BharatNarasimman
Copy link
Contributor

Yes, what we have is the requirement - min resources needed to provide the intended functionality. Since the developer hands off the component.yaml to the operator, expecting that the developer specifies the minimum's needed to make the service functional is correct IMO.

I agree that developer can default to higher than necessary values, but I think at that point this becomes sort of similar to the developer writing bad/ really inefficient code for his service.

@vturecek
Copy link
Member

The developer probably has the best understanding of minimums required. For example, as a developer I might pull in some libraries that specify a minimum memory requirement. This concept can be extended to other hardware requirements as well. For example, as a developer, if I use a camera access library, I know that my component needs camera hardware access (although I'm not sure how this could translate to minimums, except by some custom-defined resource requirements - maybe we need a more structured extension system for this).

Essentially it's the developer declaring, optionally, that this component will not work work unless you give me these minimums. I think that's an important statement for a developer to be able to make, optionally.

@iyyappam
Copy link
Contributor

iyyappam commented Oct 24, 2019

On the flip side of it, the operator should be able to override the minimum requirement provided in the spec since the operator would in best position to determine the scale of the workload and the environment. Also, the developer specifies only the minimum requirement, not the real requirement. Based on this, we should provide a way for operator to override the requirements in the ops config even when the requirements in the spec are not exposed as a parameter.

@resouer
Copy link
Member

resouer commented Nov 7, 2019

the operator would in best position to determine the scale of the workload and the environment.

@iyyappam Ops will use a "Resource" or "Scaler" Trait to achieve this. I'd suggest leave the flexibility to ops capabilities rather than simply making this filed overwritable .

To wrap up with my thoughts:

  1. resources section is needed for Dev to claim minimum/suggested resource for this app.
  2. resources section does not have to be required (i.e. Dev just don't care it). In this case, the implementation should decide how to handle it, e.g. container based platform could leave it as is but VM based platform will need to provide some default values.
  3. Ops use Traits to overwrite resource is the way I recommend. Thus the trait could either be as simple as static value, or sophisticated like ML generated resource boundary.

@BharatNarasimman I believe the practice is relying on "Resource" or "Scaler" Trait to step in. Default value is only "Back-up Plan" for platforms require explicit resource boundary set (e.g. VM). We should claim it better in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resources is optional, but if specified, requires CPU and memory
6 participants