✨ Add ability to add new data disks to VM during clone process #3214
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
|
Welcome @vr4manta! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @vr4manta. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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-sigs/prow repository. |
k8s-ci-robot
added
the
size/L
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
/test help |
|
@chrischdi: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
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-sigs/prow repository. |
|
/test pull-cluster-api-provider-vsphere-e2e-vcsim-govmomi-main |
vr4manta
force-pushed
the
multi_disk
branch
7 times, most recently
from
9c0f201
to
1636c16
Compare
|
/test pull-cluster-api-provider-vsphere-e2e-vcsim-govmomi-main |
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
@neolit123 |
vr4manta
changed the title
|
cc @lubronzhan @neolit123 @rikatz , this one should be ready for review (xref CAPV office hours) :-) Thanks @vr4manta for driving this! |
|
/assign @neolit123 @lubronzhan |
| units := make([]bool, 30) | ||
|
|
||
| for i := 0; i < int(offset); i++ { |
There was a problem hiding this comment.
what is the significance of 30 here?
offset is of type int32, isn't doing len(offset) returning an error here?
invalid argument: offset (variable of type int32) for built-in len
There was a problem hiding this comment.
This was a copy of similar logic from govmomi project: https://github.com/vmware/govmomi/blob/2e77836caee4e65547b8b27a56e246c549757d37/object/virtual_device_list.go#L483
There was a problem hiding this comment.
they are doing this though:
units := make([]bool, 30)
for i := 0; i < offset; i++ {
units[i] = true
}
i think len(offset) would trigger a compiler error.
There was a problem hiding this comment.
I think what we are wanting to do here is two things.
1.) create an array that is the max number of units any controller can hold (or maybe the one we are targeting, but not sure how to figure that). I think I read that vSphere for some controllers max is 16, but I think special cases are much much higher. We can adjust what the default array size is here to the max of any, but will leave future issues popping up where we now exceeded that controller.
2.) The loop using offset is just gonna prefill the array up to the offset point as true as to prevent us from assigning an index below that for the current disk we are attempting to add / create. This loop seems to match both govmomi and this function. The govmomi is doing len(offset) because their function is using int32 as type for offset, but mine is just int.
The main function that calls this one, we default the starting point to 0 (which should always be the primary disk). After that, its any additional disks in the vSphere OVA template or previously added data disks. If we have more disks being added then supported, I do want to make sure that something triggers a failure. We could add a check here to error out if offset is > 30 and state it exceeds the max unit number.
I am happy for any suggestions. This is probably a weak spot, but for the average user of adding 1-5 extra disks (i'm just guessing 5 is max we want to add for each node for OS usage that is not included w/ any of the PVs) I think 30 is an ok number to default with initially. What do you think?
There was a problem hiding this comment.
create an array that is the max number of units any controller can hold
aren't there exported constants for that? would the govmomi maintainers know?
i think adding a few comment in the code to clarify the choices of 30 and the rest of the logic will clarify the picture for readers. but we should avoid panics if arrays are accessed out of bounds.
my main point was that the code as is cannot work. please try this:
https://go.dev/play/p/fWud2MEYR0d
There was a problem hiding this comment.
Sounds good. I'll update the file w/ comments on the max. I'll make 30 a const that we reference to also aid in that clarity. I'll also look at that logic fix.
For my future reference, 30 is max and its documented here (30 for SATA, 16 for SCSI): Doc
According to available information, a VMware vSphere virtual machine can typically support up to four storage controllers (like SATA, SCSI, or NVMe), with each controller allowing for a maximum of 30 virtual disks per controller, depending on the controller type and vSphere version; meaning the maximum number of virtual disks per VM could be around 120 across all controllers.
There was a problem hiding this comment.
i think docs.vmware.com might be going down in a few days due to broadcom docs migration, but you can still link that URL and a quote from it. if someone needs the original page they can use web archived version.
There was a problem hiding this comment.
ah good point. I'll see if I can find the broadcom equivelent. I keep forgetting that things are in flux with these sites.
| } | ||
|
|
||
| // assignUnitNumber assigns a controller unit number to a device. | ||
| func assignUnitNumber(ctx context.Context, device types.BaseVirtualDevice, existingDevices object.VirtualDeviceList, newDevices []types.BaseVirtualDeviceConfigSpec, controller types.BaseVirtualController, offset int32) { |
There was a problem hiding this comment.
can this function be unit tested explicitly?
There was a problem hiding this comment.
Sure. I'll work on making some tests for this method.
There was a problem hiding this comment.
I've added some tests around this method. I think its covering most scenarios.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
vr4manta
force-pushed
the
multi_disk
branch
3 times, most recently
from
e3c9be0
to
78a0873
Compare
k8s-ci-robot
added
size/XL
vr4manta
force-pushed
the
multi_disk
branch
2 times, most recently
from
770f802
to
6649b60
Compare
What this PR does / why we need it:
Add API changes that adds the ability to add new data disks to VMs during the cloning process. These new disks are not present in the vSphere VM Template.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #3213