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.

Sign up for GitHub

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

Jump to bottom

OCPBUGS-13830: Add support for overriding container resources #136

Merged
merged 3 commits into from
Aug 22, 2023
Merged

OCPBUGS-13830: Add support for overriding container resources #136

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c0ffa37
api: Add OverrideResources field to DeploymentConfig
arkadeepsen Aug 4, 2023
2bae4c7
Support override of resources of cert-manager, cert-manager-cainjecto…
arkadeepsen Aug 4, 2023
01e072e
Add validateResources function and add unit test for the same. Remove…
arkadeepsen Aug 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 19 additions & 0 deletions api/operator/v1alpha1/certmanager_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,25 @@ type DeploymentConfig struct {
// +kubebuilder:validation:Optional
// +optional
OverrideLabels map[string]string `json:"overrideLabels,omitempty"`

// +kubebuilder:validation:Optional
// +optional
OverrideResources CertManagerResourceRequirements `json:"overrideResources,omitempty"`
}

// CertManagerResourceRequirements describes the compute resource requirements for the cert-manager operands,
// namely the controller, webhook and cainjector.
type CertManagerResourceRequirements struct {
// Limits describes the maximum amount of compute resources allowed.
// More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/
// +optional
Limits corev1.ResourceList `json:"limits,omitempty" protobuf:"bytes,1,rep,name=limits,casttype=ResourceList,castkey=ResourceName"`
// Requests describes the minimum amount of compute resources required.
// If Requests is omitted for a container, it defaults to Limits if that is explicitly specified,
// otherwise to an implementation-defined value.
// More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/
// +optional
Requests corev1.ResourceList `json:"requests,omitempty" protobuf:"bytes,2,rep,name=requests,casttype=ResourceList,castkey=ResourceName"`
}

// CertManagerStatus defines the observed state of CertManager
Expand Down
31 changes: 31 additions & 0 deletions api/operator/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion bundle/manifests/cert-manager-operator.clusterserviceversion.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,7 +664,10 @@ spec:
image: openshift.io/cert-manager-operator:latest
imagePullPolicy: Always
name: cert-manager-operator
resources: {}
resources:
Copy link
Member

Choose a reason for hiding this comment

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

IIUC, by default the operator would end up adding resources.requests with CPU 10m and 32Mi RAM, but no resources.limits for now.

Copy link
Member

Choose a reason for hiding this comment

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

Just noting to myself.

Copy link
Member

Choose a reason for hiding this comment

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

Is the operator container resource requests/limits customizable in any way?
We're setting it to these values here in the CSV which is a default. Asking this because, the original bug report is also about the operator resources and not just for operand.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are only setting the requests so should be ok, but yes limits need to be customizable. Can be done using subscription.spec.config so not an issue.

Copy link
Member

@swghosh swghosh Aug 2, 2023 •
edited

Choose a reason for hiding this comment

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

Ok, we need to document it later (like we did for logLevels).
+doc-req, cc: @xenolinux

requests:
cpu: 10m
memory: 32Mi
securityContext:
allowPrivilegeEscalation: false
capabilities:
Expand Down
84 changes: 84 additions & 0 deletions bundle/manifests/operator.openshift.io_certmanagers.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,34 @@ spec:
additionalProperties:
type: string
type: object
overrideResources:
description: CertManagerResourceRequirements describes the compute
resource requirements for the cert-manager operands, namely
the controller, webhook and cainjector.
properties:
limits:
additionalProperties:
anyOf:
- type: integer
- type: string
pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
x-kubernetes-int-or-string: true
description: 'Limits describes the maximum amount of compute
resources allowed. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/'
type: object
requests:
additionalProperties:
anyOf:
- type: integer
- type: string
pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
x-kubernetes-int-or-string: true
description: 'Requests describes the minimum amount of compute
resources required. If Requests is omitted for a container,
it defaults to Limits if that is explicitly specified, otherwise
to an implementation-defined value. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/'
type: object
type: object
type: object
controllerConfig:
description: "ControllerConfig specifies further customization options
Expand Down Expand Up @@ -298,6 +326,34 @@ spec:
additionalProperties:
type: string
type: object
overrideResources:
description: CertManagerResourceRequirements describes the compute
resource requirements for the cert-manager operands, namely
the controller, webhook and cainjector.
properties:
limits:
additionalProperties:
anyOf:
- type: integer
- type: string
pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
x-kubernetes-int-or-string: true
description: 'Limits describes the maximum amount of compute
resources allowed. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/'
type: object
requests:
additionalProperties:
anyOf:
- type: integer
- type: string
pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
x-kubernetes-int-or-string: true
description: 'Requests describes the minimum amount of compute
resources required. If Requests is omitted for a container,
it defaults to Limits if that is explicitly specified, otherwise
to an implementation-defined value. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/'
type: object
type: object
type: object
logLevel:
default: Normal
Expand Down Expand Up @@ -482,6 +538,34 @@ spec:
additionalProperties:
type: string
type: object
overrideResources:
description: CertManagerResourceRequirements describes the compute
resource requirements for the cert-manager operands, namely
the controller, webhook and cainjector.
properties:
limits:
additionalProperties:
anyOf:
- type: integer
- type: string
pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
x-kubernetes-int-or-string: true
description: 'Limits describes the maximum amount of compute
resources allowed. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/'
type: object
requests:
additionalProperties:
anyOf:
- type: integer
- type: string
pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
x-kubernetes-int-or-string: true
description: 'Requests describes the minimum amount of compute
resources required. If Requests is omitted for a container,
it defaults to Limits if that is explicitly specified, otherwise
to an implementation-defined value. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/'
type: object
type: object
type: object
type: object
status:
Expand Down
84 changes: 84 additions & 0 deletions config/crd/bases/operator.openshift.io_certmanagers.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,34 @@ spec:
additionalProperties:
type: string
type: object
overrideResources:
description: CertManagerResourceRequirements describes the compute
resource requirements for the cert-manager operands, namely
the controller, webhook and cainjector.
properties:
limits:
additionalProperties:
anyOf:
- type: integer
- type: string
pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
x-kubernetes-int-or-string: true
description: 'Limits describes the maximum amount of compute
resources allowed. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/'
type: object
requests:
additionalProperties:
anyOf:
- type: integer
- type: string
pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
x-kubernetes-int-or-string: true
description: 'Requests describes the minimum amount of compute
resources required. If Requests is omitted for a container,
it defaults to Limits if that is explicitly specified, otherwise
to an implementation-defined value. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/'
type: object
type: object
type: object
controllerConfig:
description: "ControllerConfig specifies further customization options
Expand Down Expand Up @@ -299,6 +327,34 @@ spec:
additionalProperties:
type: string
type: object
overrideResources:
description: CertManagerResourceRequirements describes the compute
resource requirements for the cert-manager operands, namely
the controller, webhook and cainjector.
properties:
limits:
additionalProperties:
anyOf:
- type: integer
- type: string
pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
x-kubernetes-int-or-string: true
description: 'Limits describes the maximum amount of compute
resources allowed. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/'
type: object
requests:
additionalProperties:
anyOf:
- type: integer
- type: string
pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
x-kubernetes-int-or-string: true
description: 'Requests describes the minimum amount of compute
resources required. If Requests is omitted for a container,
it defaults to Limits if that is explicitly specified, otherwise
to an implementation-defined value. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/'
type: object
type: object
type: object
logLevel:
default: Normal
Expand Down Expand Up @@ -483,6 +539,34 @@ spec:
additionalProperties:
type: string
type: object
overrideResources:
Copy link
Member

Choose a reason for hiding this comment

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

Can we please have the API change and the implementation in separate commits?

description: CertManagerResourceRequirements describes the compute
resource requirements for the cert-manager operands, namely
the controller, webhook and cainjector.
properties:
limits:
additionalProperties:
anyOf:
- type: integer
- type: string
pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
x-kubernetes-int-or-string: true
description: 'Limits describes the maximum amount of compute
resources allowed. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/'
type: object
requests:
additionalProperties:
anyOf:
- type: integer
- type: string
pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
x-kubernetes-int-or-string: true
description: 'Requests describes the minimum amount of compute
resources required. If Requests is omitted for a container,
it defaults to Limits if that is explicitly specified, otherwise
to an implementation-defined value. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/'
type: object
type: object
type: object
type: object
status:
Expand Down
4 changes: 4 additions & 0 deletions config/manager/manager.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,5 +85,9 @@ spec:
runAsNonRoot: true
seccompProfile:
type: 'RuntimeDefault'
resources:
requests:
cpu: 10m
memory: 32Mi
serviceAccountName: controller-manager
terminationGracePeriodSeconds: 10