Skip to content

Set MaxVolumesPerNode in NodeGetInfo #240

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
Merged
merged 1 commit into from
Apr 2, 2019
Merged

Set MaxVolumesPerNode in NodeGetInfo #240

merged 1 commit into from
Apr 2, 2019

Conversation

Mockery-Li
Copy link
Contributor

Fixed: #19
Get volume limits by sending HTTP request to Google metadata server
Tested for several different machine types. But don't know how to integrate it into the current e2e test, and don't know if it is necessary.
@davidz627 @msau42

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 26, 2019
@k8s-ci-robot k8s-ci-robot requested review from jingxu97 and verult March 26, 2019 15:40
@k8s-ci-robot
Copy link
Contributor

Hi @Mockery-Li. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 26, 2019
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 27, 2019
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 27, 2019
msau42
msau42 reviewed Mar 27, 2019
View reviewed changes
msau42
msau42 reviewed Mar 27, 2019
View reviewed changes
@msau42
Copy link
Contributor

msau42 commented Mar 27, 2019

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 27, 2019
msau42
msau42 reviewed Mar 28, 2019
View reviewed changes
pkg/gce-pd-csi-driver/node.go Outdated
const (
OneCPU = 1
EightCPUs = 8
MemCutoff = 3840 // 3.75GB
Copy link
Contributor

Choose a reason for hiding this comment

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

These consts can actually be lower case since we don't export it outside

Copy link
Contributor

Choose a reason for hiding this comment

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

In GCP, GB == GiB, so this should be 3750

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it should be GiB and MiB. I tried to create a custom machine, it was *1024.

pkg/gce-pd-csi-driver/node.go Outdated
machineType := ns.MetadataService.GetMachineType()
if strings.HasPrefix(machineType, "n1-") {
volumeLimits = VolumeLimit128
} else if machineType == "f1-micro" || machineType == "g1-small" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add unit tests for all of these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will have a try

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the unit test, I add a function to set machine type for fake metaserver. I'm not sure if it's an appropriate way to do so.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 28, 2019
msau42
msau42 reviewed Mar 28, 2019
View reviewed changes
msau42
msau42 reviewed Mar 29, 2019
View reviewed changes
pkg/gce-pd-csi-driver/node.go Outdated
} else {
return volumeLimits, err
}
volumeLimits = volumeLimit128
} else {
return volumeLimits, fmt.Errorf("Unexpected machine type: %s", machineType)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this can be simplified to a case statement. Where if it's custom or n1, then it's 128. Everything else is 16. I think we don't need to error for unexpected machine type. That will hopefully make this more resilient to any new machine types added in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it makes sense

@msau42
Copy link
Contributor

msau42 commented Apr 1, 2019

This is looking great! Thanks for all your work! Can you squash your commits, and also add a test case here: https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/blob/master/test/e2e/tests/single_zone_e2e_test.go. This test suite runs as part of pull-gcp-compute-persistent-disk-csi-driver-e2e

@Mockery-Li
Set MaxVolumesPerNode in NodeGetInfo
8e7c69a 
Add unit test

add e2e test case
@Mockery-Li
Copy link
Contributor Author

Test case is added to e2e test. I used the forced-push after rebasing, is it the usual way to do so?

@msau42
Copy link
Contributor

msau42 commented Apr 2, 2019

I don't know why, but for some reason go test is not picking up the new test case :-/ I'm trying out the change locally and am having no luck

@msau42
Copy link
Contributor

msau42 commented Apr 2, 2019

I tested out the driver manually on a cluster, and I see that the NodeGetInfo is returning the correct response:

I0402 22:36:59.949024       1 utils.go:52] GRPC call: /csi.v1.Node/NodeGetInfo
I0402 22:36:59.949048       1 utils.go:53] GRPC request: {}
I0402 22:36:59.949724       1 node.go:288] NodeGetInfo called with req: &csi.NodeGetInfoRequest{XXX_NoUnkeyedLiteral:struct {}{}, XXX_unrecognized:[]uint8(nil), XXX_sizecache:0}
I0402 22:36:59.949774       1 utils.go:58] GRPC response: {"accessible_topology":{"segments":{"topology.gke.io/zone":"us-central1-b"}},"max_volumes_per_node":128,"node_id":"projects/msau-k8s-dev/zones/us-central1-b/instances/e2e-test-msau-minion-group-m9td"}

And the Kubernetes Node object shows the correct entry:

    allocatable:
      attachable-volumes-csi-pd.csi.storage.gke.io: "128"
      attachable-volumes-gce-pd: "64"
      cpu: "2"
      ephemeral-storage: "91117161526"
      hugepages-2Mi: "0"
      memory: 7408816Ki
      pods: "110"

@msau42
Copy link
Contributor

msau42 commented Apr 2, 2019

/lgtm
/approve
I don't know why the new e2e test is not getting run, but we can debug that later.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 2, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Mockery-Li, msau42

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 Apr 2, 2019
@k8s-ci-robot k8s-ci-robot merged commit e717cb9 into kubernetes-sigs:master Apr 2, 2019
@msau42 msau42 mentioned this pull request Apr 2, 2019
Open
@Mockery-Li Mockery-Li deleted the volume-limits branch April 3, 2019 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msau42 msau42 msau42 left review comments

@jingxu97 jingxu97 Awaiting requested review from jingxu97

@verult verult Awaiting requested review from verult

Assignees

@msau42 msau42

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Mockery-Li @k8s-ci-robot @msau42