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

Enable volume limit feature by default #69225

Merged
merged 2 commits into from
Oct 3, 2018

Conversation

gnufied
Copy link
Member

@gnufied gnufied commented Sep 28, 2018

Also add more tests for it.

Fixes #69224

/sig storage

cc @saad-ali @msau42

Enable AttachVolumeLimit feature

@k8s-ci-robot k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 28, 2018
attachKey := v1.ResourceName(util.GetCSIAttachLimitKey(tc.driverName))
attachLimit := volumeLimits[attachKey]
if attachLimit != tc.expectedVolumeLimit {
t.Errorf("expected volume limit to be %d got %d", tc.inputVolumeLimit, attachLimit)
Copy link
Member

Choose a reason for hiding this comment

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

The log message is incorrect - expected volume limit is tc.expectedVolumeLimit not tc.inputVolumeLimit

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch.

@gnufied gnufied force-pushed the volume-limit-tidy-up branch 3 times, most recently from f62addb to d690511 Compare September 28, 2018 20:39
@msau42
Copy link
Member

msau42 commented Sep 28, 2018

Can we also add an e2e test? I don't know if this is something that unit tests would have caught.

existingNode: generateNode(
nil, /*nodeIDs*/
nil, /*labels*/
map[v1.ResourceName]resource.Quantity{
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a testcase for deletion against a node that previously doesn't contain attach limit info?

Copy link
Member Author

Choose a reason for hiding this comment

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

Now that we run the test in all cases, I think this should be covered by that

inputNodeID: "",
expectFail: true,
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a test case with pre-existing attach limit on the node?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -627,6 +658,17 @@ func test(t *testing.T, addNodeInfo bool, csiNodeInfoEnabled bool, testcases []t
continue
}

// We are testing max volume limits
if tc.inputVolumeLimit > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Is it possible to have the logic below run for every test case, and have getVolumeLimits() return 0 if the attachKey doesn't exist? IMO, intuitively setting inputVolumeLimit in a test case means this value is passed into the method being tested as an input, whereas RemoveInfo doesn't depend on that param.

@neolit123
Copy link
Member

thanks for addressing!
/kind bug

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Sep 30, 2018
@k8s-ci-robot k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Oct 1, 2018
@gnufied
Copy link
Member Author

gnufied commented Oct 1, 2018

@msau42 @verult fixed. Also added a small e2e to make sure node has limits. PTAL

if len(nodeList.Items) == 0 {
framework.Failf("Unable to find ready and schedulable Node")
}
node := nodeList.Items[0]
Copy link
Member

Choose a reason for hiding this comment

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

test description says that it checks all nodes, but we're only checking one.

Copy link
Member

Choose a reason for hiding this comment

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

should the test check all nodes?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

volumeLimits := getVolumeLimit(&node)
if len(volumeLimits) == 0 {
framework.Failf("Expected volume limits to be set")
}
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 also launch a pod that uses the entire volume limit? You could potentially reuse this test case and just set a different numVolumes: https://sourcegraph.com/github.com/kubernetes/kubernetes/-/blob/test/e2e/storage/persistent_volumes.go#L319

Copy link
Member Author

Choose a reason for hiding this comment

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

We could but for gce - it appears that the limit on the selected node starts at 64. So, we will have to create a pod with 64+ volumes

Copy link
Member

Choose a reason for hiding this comment

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

64 1gi volumes should be within our quota...

Copy link
Member Author

@gnufied gnufied Oct 1, 2018

Choose a reason for hiding this comment

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

okay, I think that will also make this test serial because no other workload(guess just with volumes but we don't have that level of granularity) can be scheduled on the node while this test runs on it. I can do that as a follow up PR while fixing #68912

@bertinatto
Copy link
Member

/retest

@bertinatto
Copy link
Member

bertinatto commented Oct 2, 2018

Just for the record, as I can't /lgtm:

I went through the code and it seems OK to me; and the unit test seems to have good coverage as well. +1 for another test case that extrapolates the volume limit.

)
f := framework.NewDefaultFramework("volume-limits-on-node")
BeforeEach(func() {
framework.SkipUnlessProviderIs("aws", "gce")
Copy link
Member

Choose a reason for hiding this comment

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

can you include "gke" in this list

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@msau42
Copy link
Member

msau42 commented Oct 2, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 2, 2018
@saad-ali
Copy link
Member

saad-ali commented Oct 2, 2018

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnufied, msau42, saad-ali

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 2, 2018
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 2d67d78 into kubernetes:master Oct 3, 2018
k8s-ci-robot added a commit that referenced this pull request Oct 4, 2018
…5-upstream-release-1.12

Automated cherry pick of #69225: Enable volume limit feature by default
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants