Reduce volume name length for vsphere #96533
Conversation
k8s-ci-robot
added
sig/storage
|
/kind bug |
k8s-ci-robot
added
kind/bug
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gnufied 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
|
/assign @jsafrane @divyenpatel |
|
/assign @misterikkit @yuga711 |
|
@msau42: GitHub didn't allow me to assign the following users: yuga711. Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
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. |
|
/milestone v1.20 |
|
/retest |
| // and systemd has a limit of 256 chars in a unit name - https://github.com/systemd/systemd/pull/14294 | ||
| // so if we subtract the kubelet path prefix from 256, we are left with 191 characters but because some characters | ||
| // will be escaped we are deliberating choosing a lower value of 180. | ||
| name := volumeutil.GenerateVolumeName(v.options.ClusterName, v.options.PVName, 180) |
There was a problem hiding this comment.
IMO the name must be much shorter than 180. We add [<datastore>] - <uuid>/ to it, where every "[]-" character is escaped as 4 characters by systemd.
For example, if we leave 20 characters for datastore names, such volume is mounted to
/var/lib/kubelet/plugins/kubernetes.io/vsphere-volume/mounts/[12345678901234567890] 31dc869f-aae9-4580-9d63-9461241123b0/<name>
That leaves 109 characters for <name> itself. Would be 90 a better number? That allows users to use datastore names with 39 characters (or less, if they use dashes that are escaped).
There was a problem hiding this comment.
+1 on truncating the name a bit more (as we have the uuid) and leave more room to the datastore name
There was a problem hiding this comment.
Is GenerateVolumeName only called on Create? Only updating the expected name here can break compatibility with other callers that expect 255 characters still.
There was a problem hiding this comment.
Yes it is only called on create and nowhere else. Updated the length to be 90 characters.
There was a problem hiding this comment.
Does changing the name here impact things like reconstruction/upgrades?
There was a problem hiding this comment.
nope. The volumeName is stored and always retrieved from PV and since we aren't changing name of existing PVs we should be fine.
|
/assign @yastij @andrewsykim |
gnufied
force-pushed
the
reduce-vsphere-volume-name
branch
from
01ff7e5 to
742436c
Compare
k8s-ci-robot
added
do-not-merge/hold
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
Reduce length of volume name for vsphere, so as not to make mount paths longer than necessary. This does not guarantee that volume path will be shorter than 180 chars but it will drop the volume prefix if volume name happens to be longer than 180 chars.
/sig storage