✨ Add ability to add new data disks to VM during clone process #3214

vr4manta · 2024-10-03T13:27:15Z

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

k8s-ci-robot · 2024-10-03T13:27:24Z

Welcome @vr4manta!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-vsphere 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-vsphere has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-10-03T13:27:25Z

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 /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-sigs/prow repository.

chrischdi · 2024-10-04T09:44:34Z

/ok-to-test

chrischdi · 2024-10-09T12:42:06Z

/test help

k8s-ci-robot · 2024-10-09T12:42:08Z

@chrischdi: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-cluster-api-provider-vsphere-e2e-vcsim-govmomi-main
/test pull-cluster-api-provider-vsphere-e2e-vcsim-supervisor-main
/test pull-cluster-api-provider-vsphere-test-main
/test pull-cluster-api-provider-vsphere-verify-main

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-vsphere-apidiff-main

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-vsphere-apidiff-main
pull-cluster-api-provider-vsphere-test-main
pull-cluster-api-provider-vsphere-verify-main

In response to this:

/test help

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.

chrischdi · 2024-10-09T12:42:17Z

/test pull-cluster-api-provider-vsphere-e2e-vcsim-govmomi-main
/test pull-cluster-api-provider-vsphere-e2e-vcsim-supervisor-main

neolit123

please ping once this is ready for review.

vr4manta · 2024-10-17T18:30:20Z

/test pull-cluster-api-provider-vsphere-e2e-vcsim-govmomi-main

vr4manta · 2024-10-17T19:55:04Z

@neolit123
This is ready to start reviewing.

chrischdi · 2024-10-23T12:38:03Z

cc @lubronzhan @neolit123 @rikatz , this one should be ready for review (xref CAPV office hours) :-) Thanks @vr4manta for driving this!

vr4manta · 2024-10-24T11:08:17Z

/assign @neolit123 @lubronzhan

neolit123

thanks for the PR @vr4manta
overall seems fine. i've added a few comments around the API and docs/code-comments here and there.

apis/v1beta1/types.go

docs/proposal/20241003-data-disk-support.md

pkg/services/govmomi/vcenter/clone.go

k8s-ci-robot · 2024-10-24T14:02:11Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from neolit123. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vr4manta · 2024-11-25T15:31:00Z

@neolit123 @lubronzhan lots of updates are in. Ready for next review. Thanks!

Also, e2e is in, but for some reason, it is not working against the e2e cluster in other PR. When I run these tests locally against my vcenter, it all seems to work. I cannot access the logs from the e2e run due to what looks like IP / VPN. Is someone able to check that and help me diagnose it? Thanks!

apis/v1beta1/types.go

pkg/services/govmomi/vcenter/clone.go

chrischdi · 2024-11-25T15:48:02Z

test/e2e/multi-disk_test.go

+	ToVersion string
+}
+
+var _ = Describe("Multi-Disk support", func() {


Suggested change

var _ = Describe("Multi-Disk support", func() {

var _ = Describe("Ensure govmomi mode is able to add additional disks to VMs [vcsim]", func() {

Can we run this with vcsim and properly name the test?

I would assume it can also run with vsim. I'll verify that is working. I'll also update the name.

It seems that using finder against vcsim doesn't like finding the VMs for some reason. The test works fine against real vCenter, but getting a bad error when trying to work against vcsim. I'm still looking into it, but I may just avoid vcsim for now.

pkg/services/govmomi/vcenter/clone.go

chrischdi · 2024-11-25T15:56:44Z

pkg/services/govmomi/vcenter/clone.go

+	}
+
+	// If we are here, we did not find a unit number.  Return error.
+	return errors.Errorf("unable to find available unit number")


Suggested change

return errors.Errorf("unable to find available unit number")

return errors.New("all unit numbers are already in-use")

Not sure if that's a good error message though.

Yeah, I would almost think the template image being already full before attempting to add an additional should never happen, but this is a good safety net in case something strange happens. I was almost leaning towards `no unit numbers are available for use", but if your suggestion makes sense to you, I am ok with it. I'll change this to the new text unless someone else has a better suggestion.

pkg/services/govmomi/vcenter/clone.go

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 3, 2024

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 3, 2024

k8s-ci-robot requested review from chrischdi and killianmuldoon October 3, 2024 13:27

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 3, 2024

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 Oct 4, 2024

This was referenced Oct 9, 2024

✨ Add ability to add new data disks to VM during clone process team-cluster-api/cluster-api-provider-vsphere#29

Open

Add data disks not present in VM template to VM during cloning #3213

Open

neolit123 reviewed Oct 10, 2024

View reviewed changes

vr4manta force-pushed the multi_disk branch 7 times, most recently from 9c0f201 to 1636c16 Compare October 17, 2024 18:13

vr4manta marked this pull request as ready for review October 17, 2024 19:38

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 17, 2024

k8s-ci-robot requested review from rvanderp3 and zhanggbj October 17, 2024 19:38

k8s-ci-robot assigned lubronzhan and neolit123 Oct 24, 2024

neolit123 reviewed Oct 24, 2024

View reviewed changes

vr4manta force-pushed the multi_disk branch 3 times, most recently from e3c9be0 to 78a0873 Compare October 29, 2024 14:37

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 29, 2024

vr4manta force-pushed the multi_disk branch 3 times, most recently from 6649b60 to f11b363 Compare November 4, 2024 14:54

Proposal for multi disk support

e31580e

vr4manta force-pushed the multi_disk branch 7 times, most recently from 4403b59 to 84cddbd Compare November 22, 2024 16:43

chrischdi reviewed Nov 25, 2024

View reviewed changes

apis/v1beta1/types.go Show resolved Hide resolved

chrischdi reviewed Nov 25, 2024

View reviewed changes

pkg/services/govmomi/vcenter/clone.go Outdated Show resolved Hide resolved

vr4manta force-pushed the multi_disk branch from 2d8543a to 61ef08b Compare November 25, 2024 17:29

vr4manta added 2 commits November 26, 2024 15:41

Added multi disk support

e0d6a79

E2E tests

fd1aaee

vr4manta force-pushed the multi_disk branch from e85ee10 to fd1aaee Compare November 26, 2024 20:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add ability to add new data disks to VM during clone process #3214

✨ Add ability to add new data disks to VM during clone process #3214

vr4manta commented Oct 3, 2024

k8s-ci-robot commented Oct 3, 2024

k8s-ci-robot commented Oct 3, 2024

chrischdi commented Oct 4, 2024

chrischdi commented Oct 9, 2024

k8s-ci-robot commented Oct 9, 2024

chrischdi commented Oct 9, 2024

neolit123 left a comment

vr4manta commented Oct 17, 2024

vr4manta commented Oct 17, 2024

chrischdi commented Oct 23, 2024

vr4manta commented Oct 24, 2024

neolit123 left a comment

k8s-ci-robot commented Oct 24, 2024

vr4manta commented Nov 25, 2024

chrischdi Nov 25, 2024

vr4manta Nov 25, 2024

vr4manta Nov 26, 2024

chrischdi Nov 25, 2024

vr4manta Nov 25, 2024

	var _ = Describe("Multi-Disk support", func() {
	var _ = Describe("Ensure govmomi mode is able to add additional disks to VMs [vcsim]", func() {

	return errors.Errorf("unable to find available unit number")
	return errors.New("all unit numbers are already in-use")

✨ Add ability to add new data disks to VM during clone process #3214

Are you sure you want to change the base?

✨ Add ability to add new data disks to VM during clone process #3214

Conversation

vr4manta commented Oct 3, 2024

k8s-ci-robot commented Oct 3, 2024

k8s-ci-robot commented Oct 3, 2024

chrischdi commented Oct 4, 2024

chrischdi commented Oct 9, 2024

k8s-ci-robot commented Oct 9, 2024

chrischdi commented Oct 9, 2024

neolit123 left a comment

Choose a reason for hiding this comment

vr4manta commented Oct 17, 2024

vr4manta commented Oct 17, 2024

chrischdi commented Oct 23, 2024

vr4manta commented Oct 24, 2024

neolit123 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 24, 2024

vr4manta commented Nov 25, 2024

chrischdi Nov 25, 2024

Choose a reason for hiding this comment

vr4manta Nov 25, 2024

Choose a reason for hiding this comment

vr4manta Nov 26, 2024

Choose a reason for hiding this comment

chrischdi Nov 25, 2024

Choose a reason for hiding this comment

vr4manta Nov 25, 2024

Choose a reason for hiding this comment