Implement dynamic volume limits#64154
Conversation
3676b0c to
2ec50f1
Compare
|
/retest |
pkg/features/kube_features.go
Outdated
There was a problem hiding this comment.
I am not sure about the feature name. DynamicVolumeLimits? AttachableVolumeLimits (= the predicate name)? There is no "Threshold" used in implementation of the feature.
There was a problem hiding this comment.
Limit is basically another word for threshold right? I just copied what was in specs. It should be pretty easy to rename the feature flag but I don't feel very strongly about this either ways.
There was a problem hiding this comment.
Since we use "Limit" in other places, I will vote for "limit". Also agree on "AttachableVolume" part
There was a problem hiding this comment.
newVolumes, err = c.filterAttachableVolumes(pod.Spec.Volumes, pod.Namespace) ?
There was a problem hiding this comment.
ah, now I know why it is written like that. It is because second execution of this function(line#452) adds volumes in a loop, we will have merge maps and iterate result again to do that. This saves some execution time.
pkg/volume/aws_ebs/aws_ebs.go
Outdated
pkg/volume/plugins.go
Outdated
There was a problem hiding this comment.
- Please add some description what is the return map - what's the key and what's value.
- I'd emphasize that it's called by kubelet and it returns limit for the node where this function is called.
pkg/volume/plugins.go
Outdated
There was a problem hiding this comment.
Add a note that this function is called by scheduler (or can be called by any Kubernetes component, not just kubelet).
6da2ccb to
9a790ce
Compare
|
i will help review kubelet changes. /assign @derekwaynecarr |
9a790ce to
75576df
Compare
There was a problem hiding this comment.
Add comment for this function?
|
cc @bsalamat Could you please help take a look at the scheduler change? Thanks! |
9fb7a39 to
a5330e3
Compare
|
For scheduler /assign @bsalamat @aveshagarwal for api and other misc. changes /assign @liggitt |
|
/test pull-kubernetes-kubemark-e2e-gce-big |
efe08a7 to
b660bb3
Compare
|
/lgtm |
|
[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process @aveshagarwal @bsalamat @derekwaynecarr @gnufied @liggitt @msau42 Pull Request Labels
|
|
/test pull-kubernetes-e2e-gce |
bsalamat
left a comment
There was a problem hiding this comment.
Scheduler changes look good to me. Just a minor comment.
pkg/apis/core/v1/helper/helpers.go
Outdated
There was a problem hiding this comment.
I would rename this to IsVolumeLimitResourceName to be consistent with the rest of the code.
define alpha feature and make api changes
Add tests for checking node limits
b660bb3 to
1f9404d
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bsalamat, derekwaynecarr, gnufied, liggitt, msau42, saad-ali The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
Automatic merge from submit-queue (batch tested with PRs 64613, 64596, 64573, 64154, 64639). If you want to cherry-pick this change to another branch, please follow the instructions here. |
| return nil, fmt.Errorf("Expected Azure cloudprovider, got %s", cloud.ProviderName()) | ||
| } | ||
|
|
||
| volumeLimits := map[string]int64{ |
There was a problem hiding this comment.
Question: @gnufied it looks like I need to write code here to query volume limits according to azure node type , and assign new value to volumeLimits in azure_dd.go, right?
There was a problem hiding this comment.
yes that is correct. I did not know how we can get those limits for Azure Disk and hence I went with existing defaults.
|
This broke the node e2e alpha suite: https://k8s-testgrid.appspot.com/sig-node-kubelet#kubelet-alpha |
|
@yuanying looking. ty |
Implement dynamic volume limits depending on node type.
xref kubernetes/community#2051