Add max number of volumes that can be attached to an instance #289
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bertinatto 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 |
k8s-ci-robot
added
the
approved
k8s-ci-robot
added
the
size/XS
Pull Request Test Coverage Report for Build 617
💛 - Coveralls |
bertinatto
force-pushed
the
add_volume_limits
branch
from
804353c
to
0b8bf4c
Compare
pkg/driver/node.go
Outdated
|
|
||
| // maxVolumesPerNode is the maximum number of volumes that an AWS instance can have attached. | ||
| // More info at https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/volume_limits.html | ||
| maxVolumesPerNode = 39 |
There was a problem hiding this comment.
Since NVME volume only allow 28 attachment, and some instance type has EBS as default volume. Feels its safer to set the value to 27 to begin with to accommodate worse case.
There was a problem hiding this comment.
I added some logic to return the volumes limit based on the instance type.
There was a problem hiding this comment.
PTAL and let me know if it makes sense.
bertinatto
force-pushed
the
add_volume_limits
branch
from
0b8bf4c
to
6201e3b
Compare
k8s-ci-robot
added
size/L
| func (d *nodeService) getVolumesLimit() int64 { | ||
| ebsNitroInstanceTypeRegex := "^[cmr]5.*|t3|z1d" | ||
| instanceType := d.metadata.GetInstanceType() | ||
| if ok, _ := regexp.MatchString(ebsNitroInstanceTypeRegex, instanceType); ok { |
There was a problem hiding this comment.
Feels its less error prune if we just maintain a hard coded list of nitro instance instead of regex. And define a function that checks if it is nitro based:
func isNitroInstance(instanceType string) boolean How do you think?
See https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/instance-types.html#ec2-nitro-instances for a full list of Nitro instances.
There was a problem hiding this comment.
I'm OK with that, but I'm not sure if I understand that page well enough to come up with a reliable list. If nothing, the regex above comes from the in-tree implementation, so it seems to be a reasonable good start.
|
Since the implementation aligns with in-tree implementation, let's merged it in. And we could refactor both later. /lgtm |
k8s-ci-robot
added
the
lgtm
Is this a bug fix or adding new feature?
/king feature
What is this PR about? / Why do we need it?
Without having this field set, k8s will interpret that the node can handle any amount of volumes. I'll create an issue to customize this number per instance type.
Closes #39
/assign @leakingtapan