Add cluster resource modeling api #2386
Conversation
karmada-bot
added
the
size/XL
|
@Poor12 This is API part of cluster resource modeling. Please help me review it. Thx. |
halfrost
force-pushed
the
cluster-resource-modeling-api
branch
from
92cce52
to
570956b
Compare
halfrost
force-pushed
the
cluster-resource-modeling-api
branch
from
54ad0db
to
bf8bf18
Compare
|
@Poor12 I have already changed the resource name into an enum. We can maintain by ourselves in the future. We can add more resource names in the enum. Please review it. |
|
@halfrost Please squash your commits and use PR templates. |
halfrost
force-pushed
the
cluster-resource-modeling-api
branch
2 times, most recently
from
d6fc8e0
to
97fd9ca
Compare
halfrost
force-pushed
the
cluster-resource-modeling-api
branch
4 times, most recently
from
a9892d2
to
0f3a668
Compare
|
/lgtm |
There was a problem hiding this comment.
Generally looks good to me.
Some small touches.
/assign @kevin-wangzefeng
halfrost
force-pushed
the
cluster-resource-modeling-api
branch
from
0f3a668
to
e76a2b7
Compare
halfrost
force-pushed
the
cluster-resource-modeling-api
branch
from
e76a2b7
to
ce8e26b
Compare
halfrost
force-pushed
the
cluster-resource-modeling-api
branch
2 times, most recently
from
dfd241c
to
928ebbd
Compare
There was a problem hiding this comment.
This is probably my last comment.
Thanks @halfrost for your hard and excellent work.
halfrost
force-pushed
the
cluster-resource-modeling-api
branch
2 times, most recently
from
3a2f317
to
1eb9380
Compare
halfrost
force-pushed
the
cluster-resource-modeling-api
branch
3 times, most recently
from
1b4c833
to
caeb54a
Compare
|
/lgtm |
| // Max is the maximum amount of this resource represented by resource name. | ||
| // Special Instructions, for the last ResourceModelRange, which no matter what Max value you pass, | ||
| // the meaning is infinite. Because for the last item, | ||
| // any ResourceModelRange's quota larger than Min will be classified to the last one. | ||
| // Of course, the value of the Max field is always greater than the value of the Min field. | ||
| // It should be true in any case. | ||
| // +optional | ||
| Max resource.Quantity |
There was a problem hiding this comment.
Special Instructions, for the last ResourceModelRange, which no matter what Max value you pass,
the meaning is infinite. Because for the last item
Shall we make the last grade ignore user-input max value, or, add a computed last/largest grade according to last/largest user-input max value?
There was a problem hiding this comment.
My current implementation is that whatever value the last max is, those greater than min will be placed in the last group. This ensures that there will be no missing ResourceModelRange. Do you think my solution is reasonable?
There was a problem hiding this comment.
I agree with the implementation behavior, I guess what @kevin-wangzefeng mean is we should clearly note the behavior on API comments.
There was a problem hiding this comment.
By the way, the behavior description should be added to ResourceModels(.spec.ResourceModels), not on ResourceModelRange which just represents the range. How we use/treat ranges is decided at ResourceModels.
pkg/apis/cluster/types.go
Outdated
| // Min is the minimum amount of this resource represented by resource name. | ||
| // +optional | ||
| Min resource.Quantity |
There was a problem hiding this comment.
Describing the default value would be very helpful here, e.g. Min of grade 0 defaults to zero
| // Grade is the index for the resource modeling. | ||
| // +optional | ||
| Grade int |
There was a problem hiding this comment.
Nit: what's the expected behavior when user sets ResourceModels with grade 1:CPU[2,4), grade 3:CPU[8,16), grade 4:CPU[16,32)?
There was a problem hiding this comment.
BTW, IIRC we currently don't define the output of kubectl describe cluster, but it might be great if we can define a human readable format of ResourceModels like:
ResourceModels:
Grade 1: CPU [2,4) Memory [4G,8G)
Grade 2: CPU [4,8) Memory [8G,16G)
Grade 3: CPU [8,16) Memory [16G,32G)
A method like ResourceModel.Stringify() shall be helpful in this feature.
There was a problem hiding this comment.
when user sets ResourceModels with grade 1:CPU[2,4), grade 3:CPU[8,16), grade 4:CPU[16,32), if some resource's CPU is 6, this resource will be assigned to a higher grade. In other words, it will belong to grade 3:CPU[8,16). Does it make sense?
By the way, in this case, grade2 doesn't exist? Or the user forgot to enter it?
There was a problem hiding this comment.
This ResourceModel.Stringify() feature is very nice. If time is enough, I can do it together in this version.
There was a problem hiding this comment.
when user sets ResourceModels with grade 1:CPU[2,4), grade 3:CPU[8,16), grade 4:CPU[16,32), if some resource's CPU is 6, this resource will be assigned to a higher grade.
+1. We can take this as the default behavior. But I don't think we should note this case.
By the way, in this case, grade2 doesn't exist? Or the user forgot to enter it?
This is an abnormal case, probably due to a mistake.
This usage is ambiguous, my suggestion is to validate the input at webhook(Validation Plugin) to avoid discontinuous ranges.
halfrost
force-pushed
the
cluster-resource-modeling-api
branch
from
caeb54a
to
ec74e48
Compare
halfrost
force-pushed
the
cluster-resource-modeling-api
branch
from
ec74e48
to
3182fc7
Compare
pkg/apis/cluster/v1alpha1/types.go
Outdated
| Name ResourceName `json:"name,omitempty"` | ||
|
|
||
| // Min is the minimum amount of this resource represented by resource name. | ||
| // E.g. Min of grade 0 defaults to zero. |
There was a problem hiding this comment.
// Note: The Min value of first grade(usually 0) always acts as zero.
// E.g. [1,2) equal to [0,2).
3182fc7
to
b72dd61
Compare
Signed-off-by: halfrost <ydz627@gmail.com>
halfrost
force-pushed
the
cluster-resource-modeling-api
branch
from
b72dd61
to
77cf4be
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RainbowMango 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 |
karmada-bot
added
the
approved
|
This is a good case for learning code review :) |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR introduced a feature of cluster resource modeling
Which issue(s) this PR fixes:
Part of #772
Special notes for your reviewer:
This pr is for issue #772 cluster resource modeling API part.
Does this PR introduce a user-facing change?: