✨ 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.

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

Looks great, I think that's the last round for me :-)

Sorry for the big suggestion, that escalated a bit quickly on my side 😂

pkg/services/govmomi/vcenter/clone.go

test/e2e/data/infrastructure-vsphere-govmomi/main/multi-disk/data-disks-patch.yaml

test/e2e/multi-disk_test.go

chrischdi · 2024-12-04T17:31:25Z

Big Kudos for the e2e test! That's a very efficient one and also easy to understand!

test/e2e/multi-disk_test.go

vr4manta · 2024-12-05T17:09:18Z

Big Kudos for the e2e test! That's a very efficient one and also easy to understand!

Thanks!

vr4manta · 2024-12-05T17:10:31Z

Looks great, I think that's the last round for me :-)

Sorry for the big suggestion, that escalated a bit quickly on my side 😂

No problem. I do like making it an object that maintains the state. I've made the changes and am testing more thoroughly.

pkg/services/govmomi/vcenter/clone.go

pkg/services/govmomi/vcenter/clone_test.go

chrischdi · 2024-12-10T09:36:20Z

I'm good with the change! Let's get this over the finish line :-)

pkg/services/govmomi/vcenter/clone_test.go

chrischdi · 2024-12-11T15:13:58Z

/approve

I'm okay to merge this.

Just want to give @neolit123 or @rikatz a chance if they want to take a look before merging.

k8s-ci-robot · 2024-12-11T15:14:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrischdi

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:

~~OWNERS~~ [chrischdi]

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 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

vr4manta force-pushed the multi_disk branch 3 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 2 times, most recently from e85ee10 to fd1aaee Compare November 26, 2024 20:41

chrischdi reviewed Dec 4, 2024

View reviewed changes

test/e2e/multi-disk_test.go Outdated Show resolved Hide resolved

neolit123 reviewed Dec 5, 2024

View reviewed changes

test/e2e/multi-disk_test.go Outdated Show resolved Hide resolved

neolit123 reviewed Dec 5, 2024

View reviewed changes

test/e2e/multi-disk_test.go Outdated Show resolved Hide resolved

Proposal for multi disk support

5cde143

vr4manta force-pushed the multi_disk branch from 810c059 to 8848cff Compare December 5, 2024 16:36

chrischdi reviewed Dec 10, 2024

View reviewed changes

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

chrischdi reviewed Dec 10, 2024

View reviewed changes

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

chrischdi reviewed Dec 10, 2024

View reviewed changes

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

Added multi disk support

4af6c63

vr4manta force-pushed the multi_disk branch from 8848cff to f622f9d Compare December 10, 2024 13:51

chrischdi reviewed Dec 10, 2024

View reviewed changes

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

Added multi disk support tests

2c30cb1

vr4manta force-pushed the multi_disk branch from f622f9d to 2c30cb1 Compare December 11, 2024 12:40

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 11, 2024

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

vr4manta commented Nov 25, 2024

chrischdi Nov 25, 2024

vr4manta Nov 25, 2024

vr4manta Nov 26, 2024

chrischdi left a comment

chrischdi commented Dec 4, 2024

vr4manta commented Dec 5, 2024

vr4manta commented Dec 5, 2024

chrischdi commented Dec 10, 2024

chrischdi commented Dec 11, 2024

k8s-ci-robot commented Dec 11, 2024

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

✨ 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

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 left a comment

Choose a reason for hiding this comment

chrischdi commented Dec 4, 2024

vr4manta commented Dec 5, 2024

vr4manta commented Dec 5, 2024

chrischdi commented Dec 10, 2024

chrischdi commented Dec 11, 2024

k8s-ci-robot commented Dec 11, 2024