-
Notifications
You must be signed in to change notification settings - Fork 74
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 qos resources for deployments managed by clustermanager and klusterlet #316
✨ Configurable qos resources for deployments managed by clustermanager and klusterlet #316
Conversation
/assign @qiujian16 |
operator/v1/types_klusterlet.go
Outdated
) | ||
|
||
// ResourceRequirement allow user override the default pod QoS classes | ||
type ResourceRequirement struct { | ||
// +kubebuilder:validation:Enum=Default;BestEffort | ||
// +kubebuilder:default:=Default | ||
Type ResourceQosClass `json:"type"` | ||
// Resources defines resource requests and limits when Type is ResourceQosClassConfigurable | ||
// +optional | ||
Resources *corev1.ResourceList `json:"resources,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would need to add ResourceQosClassConfigurable in the // +kubebuilder:validation:Enum=Default;BestEffort for type.
I would suggest the type is ResourceRequirements and the field name should be ResourceRequirements too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added ResourceQosClassConfigurable in the validation comment
I would suggest the type is ResourceRequirements and the field name should be ResourceRequirements too
Do you mean changing
ResourceRequirement *ResourceRequirement `json:"resourceRequirement,omitempty"`
to
ResourceRequirements *ResourceRequirements `json:"resourceRequirement,omitempty"`
But json:"resourceRequirement,omitempty"
has to stay unchanged, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ResourceRequirement *ResourceRequirement json:"resourceRequirement,omitempty"
is ok
operator/v1/types_klusterlet.go
Outdated
) | ||
|
||
// ResourceRequirement allow user override the default pod QoS classes | ||
type ResourceRequirement struct { | ||
// +kubebuilder:validation:Enum=Default;BestEffort | ||
// +kubebuilder:default:=Default | ||
Type ResourceQosClass `json:"type"` | ||
// Resources defines resource requests and limits when Type is ResourceQosClassConfigurable | ||
// +optional | ||
Resources *corev1.ResourceList `json:"resources,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think *corev1.ResourceList is the correct type. What you want is https://github.com/kubernetes/api/blob/master/core/v1/types.go#L2547?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you are right. Fixed.
|
||
// ResourceRequirement specify QoS classes of deployments managed by clustermanager | ||
// +optional | ||
ResourceRequirement *ResourceRequirement `json:"resourceRequirement,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be noted that it applies to all containers since there are multiple containers running in the cluster manager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment added
the deepcopy go file is not updated, is there any error durning running the |
6c35afe
to
9fa5b15
Compare
It indeeded had some errors I didn't pay attention to. Let me check and fix it |
9fa5b15
to
cc2b204
Compare
The project path was wrong. I have fixed it. deepcopy is correctly generated now. |
operator/v1/types_klusterlet.go
Outdated
ResourceQosClassDefault ResourceQosClass = "Default" | ||
// If all containers in the pod don't set resource request and limits, the pod is treated as BestEffort. | ||
ResourceQosClassBestEffort ResourceQosClass = "BestEffort" | ||
// Configurable resources with requests and limits | ||
ResourceQosClassConfigurable ResourceQosClass = "Configurable" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the type name should be ResourceRequirement
ResourceQosClassResourceRequirement ResourceQosClass = "ResourceRequirement"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
cc2b204
to
f0a8b03
Compare
operator/v1/types_klusterlet.go
Outdated
// +kubebuilder:default:=Default | ||
Type ResourceQosClass `json:"type"` | ||
// Resources defines resource requests and limits when Type is ResourceQosClassResourceRequirement | ||
// +optional | ||
Resources *corev1.ResourceRequirements `json:"resources,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resources *corev1.ResourceRequirements `json:"resources,omitempty"` | |
ResourceRequirement *corev1.ResourceRequirements `json:"resourceRequirement,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this change is made, the yaml will look like:
resourceRequirement:
resourceRequirement:
limits:
...
duplicated resourceRequirement
s look weild I suppose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In kubernetes Container struct, it is
Resources ResourceRequirements `json:"resources,omitempty" protobuf:"bytes,8,opt,name=resources"`
https://github.com/kubernetes/api/blob/master/core/v1/types.go#L2682
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is intended to follow the union pattern https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/1027-api-unions/README.md. So either the type should be Resources or the filed name should be ResourceRequirement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
fd5489a
to
f0a8b03
Compare
the DCO Check requires a Signed-off-by trailer in the commit log. so you need to add |
f0a8b03
to
3e5a4c0
Compare
Done |
Signed-off-by: Dong Beiqing <350758787@qq.com>
e303165
to
ee4bb4c
Compare
b15428a
to
15ae4da
Compare
Signed-off-by: Dong Beiqing <350758787@qq.com>
15ae4da
to
25e246a
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: promid, qiujian16 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
e7bd1bd
into
open-cluster-management-io:main
Summary
Support configurable qos resources for deployments managed by clustermanager and klusterlet because "Default" deployment resources w/ only hard coded "resources" but no "limits" is not good enough and "BestEffort" is usually not allowed in production.
Related issue(s)
Fixes #