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

Configurable resource requirements for APIManager components #454

Merged
merged 5 commits into from
Sep 1, 2020

Conversation

miguelsorianod
Copy link
Contributor

@miguelsorianod miguelsorianod commented Aug 20, 2020

This PR adds configurable resource requirements for APIManager components.

Summary of the implemented behaviour

  • Configurable resource requirements per component has been added
  • When resource requirements are not specified at component level the global defaults for the component are used for it (if enabled)
  • When resource requirements are specified at component level, they take over preference over the global default and they follow a REPLACE behaviour (instead of merge behaviour) with the defaults.

Detailed implemented behaviour

  • global-level defaults for resource limits and requests for the components of 3scale are provided. This global-level defaults can be enabled or disabled through a CR attribute that already existed before this PR, named 'resourceRequirementsEnabled'.
  • component-level resources attribute is provided at component level (for example spec.backend.listenerSpec.resources attribute):
  • If it is not set by the user or set to nil and the global-level CR attribute is not set to true then no resource limits and/or requests are enforced
  • If it is not set by the user or set to nil and the global-level CR attribute is set to true then the defaults for that given component are set
  • If it is set by the user to a non-nil value then the resources requests and limits set are the ones specified by the user with a REPLACE behaviour, not a merge behavior. Global defaults are not used for that component then, independently of the value of the global-level CR attribute. In this case, omitting a limit, request or cpu,memory inside a limit o request is equivalent to not enforcing it for that component. This is the same behavior that ResourceLimits have in Kubernetes.

Possible use case scenarios

  1. The user wants to have default limits and requests for all components enabled and wants to modify them for a specific DeploymentConfig
  • The user is able to do that by setting the global resourceRequirementsEnabled attribute to true (default behavior) and then specifying the desired
    values on the corresponding resources attribute provided at component level in the APIManager CR
  1. The user wants to have default limits and requests for all components disabled and wants to set them with custom values for a specific DeploymentConfig
  • The user is able to do that by setting the global resourceRequirementsEnabled attribute to false and then specifying the desired values on the
    corresponding resources attribute provided at component level in the APIManager CR
  • Limitation: The user has no way to specify not having a default limit and having a custom request for that same component or viceversa
  1. The user wants to have default limits and requests for all components enabled and wants to disable them for a specific DeploymentConfig
  • The user is able to do that by setting the global resourceRequirementsEnabled attribute to true (default behavior) and then setting the corresponding
    resources attribute provided at component level in the APIManager CR as the empty map value {}

Unsupported scenarios

Basically the ones that involve a potentially desired merge behavior between global defaults and custom values:

  1. On a given component, having a global default resource request and a custom resource limit for that same component or viceversa
  2. On a given component, not having a global default resource request and having a custom resource limit for the same component or viceversa
  3. On a given component, having a global default CPU and having custom Memory, or having global default Memory and having custom CPU for resource requirements
  4. On a given component, having a global default CPU and having custom Memory, or having global default Memory and using custom CPU for resource limits

Allowing those unsupported scenarios would require us to stop using the v1.ResourceRequirements type provided by Kubernetes in our CR and provide our own type where we can differentiate between setting resource requests to nil or empty, as the Kubernetes implementation treats those values with the same behavior, which is not enforcing them in that case.
The alternative implementation:

  • Increases complexity
  • May not be intuitive for users as it differs from how v1.ResourceRequirements field works (nil, vs empty, ...)
  • Removes flexibility. Right now by reusing v1.ResourceRequirements other possible additional resources can be specified potentially, we would not be limited to CPU and requests. We would not be able to take advantage on changes/improvements on that type
  • Allows more configuration combinations between defaults and custom resources at both requests/limits level and cpu/memory inside requests/limits
  • Opens up new additional questions about how to specify cpus and memory and other potential resources (provide a map as kubernetes does, attribute fields? use Quantity attribute? etc...)
  • Documentation

@miguelsorianod miguelsorianod requested a review from eguzki August 20, 2020 09:54
@miguelsorianod miguelsorianod changed the title Configurable resource requirements Configurable resource requirements for APIManager components Aug 20, 2020
@eguzki
Copy link
Member

eguzki commented Aug 27, 2020

Configurable resource requirements per component has been added

👍 per component is per container.

When resource requirements are not specified at component level the global defaults for the component are used for it (if enabled)

Good. Intuitive and common sense.

When resource requirements are specified at component level, they take over preference over the global default and they follow a REPLACE behaviour (instead of merge behaviour) with the defaults.

Agree. REPLACE behavior is easier and enough. Merge behavior (with defaults) can be confusing and requires not standard data structure type in the CRD to define resource requirements. REPLACE behavior requires, though, to know existing defaults. A user may want to update cpu limit and leave cpu requests, memory limits and memory requests "as defaults". So the user needs to create structure with desired cpu limit and copy other values from defaults. For example:

resources:
      requests:
        memory: "64Mi" <- copied from defaults
        cpu: "250m" <- copied from defaults
      limits:
        memory: "128Mi" <- copied from defaults
        cpu: "500m" <- desired cpu value

For that, default values should be documented.

Copy link
Member

@eguzki eguzki left a comment

Choose a reason for hiding this comment

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

Overall LGTM.

Left some comments. I also miss some option provider tests.

@eguzki eguzki assigned miguelsorianod and unassigned eguzki Aug 27, 2020
@miguelsorianod miguelsorianod force-pushed the configurable-resource-requirements branch from 1eb1f5e to cd676ba Compare August 28, 2020 10:27
@miguelsorianod
Copy link
Contributor Author

Remanining tests added.

Documentation pending.

@miguelsorianod
Copy link
Contributor Author

Added documentation

@miguelsorianod miguelsorianod requested a review from eguzki August 28, 2020 11:20
@miguelsorianod miguelsorianod force-pushed the configurable-resource-requirements branch 2 times, most recently from 67bede6 to 4736a01 Compare August 28, 2020 11:22
doc/apimanager-reference.md Outdated Show resolved Hide resolved
@eguzki eguzki assigned miguelsorianod and unassigned eguzki Aug 28, 2020
@eguzki
Copy link
Member

eguzki commented Aug 28, 2020

fix the broken link and merge.

doc/apimanager-reference.md Outdated Show resolved Hide resolved
@miguelsorianod miguelsorianod force-pushed the configurable-resource-requirements branch from 4736a01 to cd50357 Compare August 28, 2020 14:51
DeploymentConfig-level ResourceRequirement attributes in the CR
have priority over spec.resourceRequirementsEnabled setting and
overwrite the values set by it
DeploymentConfig-level ResourceRequirement attributes in the CR
have priority over spec.resourceRequirementsEnabled setting and
overwrite the values set by it
DeploymentConfig-level ResourceRequirement attributes in the CR
have priority over spec.resourceRequirementsEnabled setting and
overwrite the values set by it
DeploymentConfig-level ResourceRequirement attributes in the CR
have priority over spec.resourceRequirementsEnabled setting and
overwrite the values set by it
@miguelsorianod miguelsorianod force-pushed the configurable-resource-requirements branch from cd50357 to 7b3bf5a Compare August 31, 2020 09:27
@codeclimate
Copy link

codeclimate bot commented Aug 31, 2020

Code Climate has analyzed commit 7b3bf5a and detected 15 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1
Duplication 14

View more on Code Climate.

@miguelsorianod miguelsorianod merged commit 2e1cec3 into master Sep 1, 2020
@miguelsorianod miguelsorianod deleted the configurable-resource-requirements branch September 1, 2020 08:44
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.

2 participants